Skip to content

fix: ensure drained() resolves after async tasks complete#89

Merged
mcollina merged 2 commits into
mcollina:masterfrom
todoroff:master
Dec 23, 2024
Merged

fix: ensure drained() resolves after async tasks complete#89
mcollina merged 2 commits into
mcollina:masterfrom
todoroff:master

Conversation

@todoroff

Copy link
Copy Markdown
Contributor

fixes #88

Comment thread queue.js Outdated
previousDrain()
resolve()
}
setImmediate(() => {

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you check if a queueMicroTask() call does the job as well? I would prefer not to turn the event loop.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you check if a queueMicroTask() call does the job as well? I would prefer not to turn the event loop.

That won't run on older versions of node though and I noticed in other PRs that you want it to be compatible all the way back essentially.

process.nextTick() does the job and is even higher priority than queueMicroTask(), so I can use that if you like? My worry was adding the ability to potentially starve the event loop, although it shouldn't be possible with normal usage.

Let me know how you'd like to proceed.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nextTick is perfect here, thanks!

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding starving the event loop, it is 100% possible with this module. Either it's a concern applied to all methods or it's not, and I prefer to leave that decision to the users.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mcollina @todoroff I was using this library in my react-native app, unfortunately q.drained() doesn't get called anymore after migrating to latest version, can we add something that works well with the other environments also, In my case process.nextTick doesn't get called in react-native.

@mcollina mcollina left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

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 this pull request may close these issues.

not waiting for async tasks to finish before resolving drained()

3 participants