fix(poller): correctly allow cancellation of timeouts after the first one #287
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… one The `stop` method cancels the timeout stored in `nextTimer` if it's set but we were only initializing it the first time we set the timeout, not for any subsequent ones. The `stopped` flag means that `callback` was not called, but this can still cause a memory leak and delay shutdown of the Node process. Assigning each timeout to the nextTimer variable ensures that the clearTimeout call works correctly to invalidate the timeout that has been set.
|
I think there is an additional race condition bug where if you call |
|
Hi, thanks for opening this PR! I opened up a separate one so I could include the changes suggested in the above comment: #292 And you can see it has now been released and I cut a new node-server-sdk version to use the update In short, please upgrade your node-server-sdk version to 3.12.1 to use the changes that you’ve proposed here |
|
Thanks to both of you! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The
stopmethod cancels the timeout stored innextTimerif it's set but we were only initializing it the first time we set the timeout, not for any subsequent ones. Thestoppedflag means thatcallbackwas not called, but this can still cause a memory leak and delay shutdown of the Node process.Assigning each timeout to the nextTimer variable ensures that the clearTimeout call works correctly to invalidate the timeout that has been set.
Tested locally in a larger project that getInstance().stopPolling() allows a Node server to shut down successfully, versus before it would wait up to 30 seconds until the next scheduled poll.
labels: mergeable, bugfix