Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Polling continues after master timeout triggers #16

Open
forivall opened this issue Sep 24, 2019 · 11 comments · May be fixed by #28
Open

Polling continues after master timeout triggers #16

forivall opened this issue Sep 24, 2019 · 11 comments · May be fixed by #28

Comments

@forivall
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Owner

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
Copy link
Contributor Author

forivall commented Sep 28, 2019

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
Copy link

justinelm commented Dec 27, 2021

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

@justinelm
Copy link

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

@justinelm
Copy link

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

@Jackman3005
Copy link

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
Copy link

@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
Copy link

@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 added a commit to questmate/promise-poller that referenced this issue Aug 27, 2022
Fixed 2 issues for the situation where a task resolves instead of rejects and the call to `shouldContinue` returns true
1. If the `masterTimeout` time has been exceeded, it will now reject the promise.
2. It will now check if there are retries remaining and reject if there are no retries remaining or else decrement remaining retries

Resolves [this issue](joeattardi#16).
@Jackman3005
Copy link

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

@jessekoconnor
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants