Is there an alternative for recursive approach I use here in node.js?

Question

I implemented a recursive function in a requestHandler I made to serialize API requests and also to make sure the endpoint isn't currently being requested. To make sure that the endpoint isn't currently being requested, I add it to a Set and verify it with conditionals.

Problem is that this recursive approach consumes quite a lot of memory when a lot of requests are made to the same endpoint. Is there any way I could make it less memory intensive as well as performant at the same time? I would love to hear any alternative approach which I could use instead of recursion. Below you can find my code.

async request(endpoint, domain, method, headers, query, body, attachments) {
    const requestURL = `${(domain === "discord") ? this.discordBaseURL :
        (domain === "trello") ? this.trelloBaseURL : domain}/${endpoint}`;

    if (this.queueCollection.has(endpoint) === false) { // queueCollection is the Set in which I store endpoints that are currently being requested by my requestHandler.
        this.queueCollection.add(endpoint);
        const response = await this.conditionalsHandler(endpoint, requestURL, method, headers, query, body, attachments);
        this.queueCollection.delete(endpoint);
        return response;
    }
    else {
        const response = new Promise((resolve) => {
            setTimeout(() => {  // https://stackoverflow.com/a/20999077
                resolve(this.request(endpoint, domain, method, headers, query, body, attachments)); // This is where I make the method recursive to call itself back until the endpoint is no longer in the queueCollection Set.
            }, 0);
        });
        return response;
    }
}

Your wall of text makes reading your question intimidating. I recommend that you simplify your question, and generalize that problem so that it's not super specific to your use case. There's a lot of noise here that makes it hard to figure out what the fundamental problem actually is. — g.delgado, Jan 11 '19 at 17:42
Is it better now? Sorry I'm new to stackoverflow so I thought adding more details could make others understand my issue — Santhosh Annamalai, Jan 11 '19 at 17:55
Do you have anything that would show that you actually have a problem to solve? — Kevin B, Jan 11 '19 at 17:56
My node process memory usage increases (from 100 - 600/700 MB) a lot if I make multiple requests to same endpoint. It doesn't happen if I make requests to different endpoints simultaneously. So I just wanted to know if there is an alternative approach I could use here instead of recursion. I tried using EventEmitter to fire an event when the request gets completed and thought of allowing the second request I make to the same endpoint after the event gets fired. I however don't know how to implement that using EventEmitter. So I just want to know if there is a better way to solve memory issue — Santhosh Annamalai, Jan 11 '19 at 18:04
@SanthoshA. imo, the first draft of your question was fine. I hope you'll find my answer to be helpful. — Patrick Roberts, Jan 11 '19 at 18:24

Patrick Roberts · Accepted Answer · 2019-01-11T19:22:39.610

Yes, you can remove the recursion by making the queueCollection a Map<string, Promise> instead of a Set<string>, and instead of recursing asynchronously and polling the queue until it's empty, chain the request to the tail of the queue if it exists like this:

async request(endpoint, domain, method, headers, query, body, attachments) {
  const requestURL = `${(domain === "discord") ? this.discordBaseURL :
    (domain === "trello") ? this.trelloBaseURL : domain}/${endpoint}`;

  // get existing queue or create a new one
  const queue = this.queueCollection.get(endpoint) || Promise.resolve();
  // schedule request on the tail of the queue
  const request = queue.then(
    () => this.conditionalsHandler(endpoint, requestURL, method, headers, query, body, attachments)
  );
  // prevent errors from propagating along the queue
  const tail = request.catch(() => {});

  // enqueue the request
  this.queueCollection.set(endpoint, tail);

  try {
    // propagates error handling to consumer
    // waits for request to settle before executing finally block
    return await request;
  } finally {
    // only remove promise from Map if this settled request is at the tail of the queue
    if (this.queueCollection.get(endpoint) === tail) this.queueCollection.delete(endpoint);
  }
}

This approach allows request to throw without breaking the chain so the consumer can handle the error and all the requests will still happen in sequence without depending on previous requests being successful, and it will always clean up the queueCollection on the last pending request regardless of whether the request throws. The await is not redundant here for that reason.

@SanthoshA. I made a slight edit. You actually need to pass a function to `.catch()` in order for it to prevent the error from propagating, otherwise it's equivalent to calling `.catch(error => { throw error })` — Patrick Roberts, Jan 11 '19 at 19:29
Thanks again! Just noticed it in my testing and fixed that issue — Santhosh Annamalai, Jan 11 '19 at 20:41

Is there an alternative for recursive approach I use here in node.js?

1 Answers1