Polling continues after master timeout triggers

[forivall]

If shouldContinue returns true, but the masterTimeout has fired, polling continues.

Let me know if you're unable to reproduce, and I'll create a test case.

[forivall]

Oh yeah, i think the solution would be to just add a check for polling at https://github.com/joeattardi/promise-poller/blob/ab3bc27/src/lib/promise-poller.js#L81

[joeattardi]

I wasn't able to reproduce this - can you provide a test case? Here is the test I tried, do I have the logic wrong?

it('does not continue polling if shouldContinue returns true after the master timeout has been triggered', done => {
    let numPolls = 0;

    const taskFn = () =>
      new Promise(function(resolve, reject) {
        numPolls += 1;
        reject('derp');
      });

    promisePoller({
      taskFn,
      masterTimeout: 500,
      shouldContinue: () => true,
      retries: 10
    }).then(
      () => {
        fail('Master promise was resolved, should have hit master timeout');
        done();
      },
      () => {
        expect(numPolls).not.toBeGreaterThan(2);
        done();
      }
    );
  });

[forivall]

As quickly as i needed promise-poller, i no longer need it (because we switched this api from a proof-of-concept polling to using websockets), but here's the code where it was being used: (the "done" is a workaround i used)

    let done = false;
    return promisePoller({
      interval: 1000,
      strategy: 'fixed-interval',
      taskFn: () =>
        promisePoller({
          interval: 1000,
          masterTimeout: 30000,
          strategy: 'exponential-backoff',
          taskFn: () => api.getStatus(),
        }),
      shouldContinue(reason, value) {
        if (reason || done) {
          return false;
        }
        /** @type {VerificationStatus} */
        const status = value.verificationStatus;
        switch (status) {
          case 'IN_PROGRESS':
          case 'NOT_STARTED':
            return true;
          case 'SUCCESS':
          case 'FAILURE':
            return false;
          default:
          /* nothing */
        }
        return false;
      },
      progressCallback(retriesRemaining, error) {
        console.info(`${retriesRemaining} retries remaining, got`, error);
      },
    }).then(
      (result) => {
        done = true;
        return result;
      },
      (reason) => {
        done = true;
        throw reason;
      }
    );

[justinelm]

*looks afraid
HAHAHAHA, hope it won't break on my app

[justinelm]

It did, oh noo. Will try to make sense of your code, Thanks!

[justinelm]

Made my own poller instead. Happy it does the work <3

[Jackman3005]

I also have this problem. I realize the issue is when you are polling an API that returns successfully each time. Two problems are occuring. One we do not stop polling if the master timeout is hit, because we do not check the polling boolean in the success case. Two, we are not decrementing the retriesRemaining in the success case.

I was intending to use the library to poll an API that gives a success response always and when the data of the response coming from that API changes to a completed state, then I stop polling and move on.

[Jackman3005]

@joeattardi If I fix the above issues in a PR. Would you be available to review, merge, and publish an update? I noticed the repo has some other PRs that haven't gone through and there have not been updates in a while. I don't want to spend time improving the library if the changes will never make it through...

[Jackman3005]

@forivall @jasonstitt Do you know the status of this any better? I am unable to find another polling library with feature parity of Promise Poller, so I'd prefer if it could be updated rather than trying to make something else work.

[Jackman3005]

@joeattardi could you please review the PR I submitted? If you are satisfied please merge & deploy an update 🙏.

[jessekoconnor]

@joeattardi - you should tag them in your actual PR as reviewers, I could use this fix too