1

I'm using node-request for sending the requests to server to get some report. The thing is server needs some time to generate the report, so it responses with the report state. I'm checking the report state with setInterval() function, and use clearInterval() when server sends ready response. But with this approach, even after I use clearInterval, responses of earlier requests keep coming, and the response handler runs again and again. This does not cause a lot of harm, but still I believe it can be done better.

Here is my code:

checkForReportReady = setInterval =>
  @request URL, options, (err, res, body) =>
    console.log err if err
    body = JSON.parse body

    if body['status'] is 'ready'
      clearInterval checkForReportReady
      @processReport body
  , 1000

What I need: make a request, wait for response, check the status, if status is not ready - make another request after some timeout, repeat until the status code in response is ready. If the status is ready - exit the loop (or clear the interval) and run @processReport.

I tried to make promisified request, and put it into setInterval, but the result was the same.

P.S. I do not control the server, so I can't change the way it responds or deals with the report.

Community
  • 1
  • 1
eawer
  • 1,398
  • 3
  • 13
  • 25

2 Answers2

1

It seems like you can just use a setTimeout() in your response handler:

function checkForReportReady() {
    request(URL, options, function(err, res, body) {
        if (err) {
            console.log(err);
        } else {
            if (body.status === "ready") {
                processReport(body);
                // do any other processing here on the result
            } else {
                // try again in 20 seconds
                setTimeout(checkForReportReady, 20*1000);
            }
        }
    });
}

This will run one request, wait for the response, check the response, then if it's ready it will process it and if it's not ready, it will wait a period of time and then start another request. It will never have more than one request in-flight at the same time.


If you want to use Bluebird promises, you can do that also, though in this case it doesn't seem to change the complexity particularly much:

var request = Promise.promisifyAll(require('request'));

function checkForReportReady() {
    return request(URL, options).spread(function(res, body) {
        if (body.status === "ready") {
            return body;
        } else {
            // try again in 20 seconds
            return Promise.delay(20 * 1000).then(checkForReportReady);
        }
    });
}

checkForReportReady().then(function(body) {
    processReport(body);
}, function(err) {
    // error here
});
jfriend00
  • 683,504
  • 96
  • 985
  • 979
  • Well it would if you'd avoid the [`Promise` constructor antipattern](http://stackoverflow.com/q/23803743/1048572) :-) – Bergi Sep 02 '15 at 12:26
1

I would recommend not to put requests in an interval callback. This can get ugly when they a) fail b) take longer than the interval.

Instead put a setTimeout in the success handler and try again after (and only if) receiving a response.
This is rather easy with promises:

request = Promise.promisifyAll require 'request'
getReport = () =>
  request URL, options
  .spread (res, body) =>
    body = JSON.parse body
    if body.status is 'ready'
      body
    else
      Promise.delay 1000
      .then getReport # try again

getReport().then(@processReport, (err) -> console.log(err))
Bergi
  • 630,263
  • 148
  • 957
  • 1,375