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

Match tick callback to Node's logic #75

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

wbyoung
Copy link

@wbyoung wbyoung commented Jan 16, 2017

Node calls the tick callback from two places: MakeCallback and AsyncWrap::MakeCallback.

Originally, I thought maybe MakeCallback could be used within deasync.cc to gain the same behavior with purely public APIs. My goal was to remove the the use of the private _tick<?Domain>Callback, but invocation of tick_callback_function is actually blocked because node will always be in a nested AsyncCallbackScope for both MakeCallback and AsyncWrap::MakeCallback. Initial code execution begins in such a context, and all future callbacks are done via subclasses of AsyncWrap, so their MakeCallback invocation always puts them back in an AsyncCallbackScope.

So since it wasn't possible to remove _tick<?Domain>Callback, I figured it would at least make sense to mirror how Node chooses between these two functions.

When domain is required it sets up domain use which changes the tick_callback_function in the Environment and also adds a domain property to process. So at the very least, I think it makes sense to invoke the callback that Node would be invoking once no longer in a nested AsyncCallbackScope.

While this doesn't really change much today since _tickCallback and _tickDomainCallback functions are nearly identical (and the domain version is graceful with the absence of a domain), it does better future-proof the code a little (especially since domains are deprecated).

@abbr
Copy link
Owner

abbr commented Jan 17, 2017

Thx for the pr. The change appears to be innocuous, but does it fix any issues you experienced, or it's just in order to match the 2 callback APIs node provided? It would be nice to identify a bug it can fix to justify the benefit outweighs the risk of causing existing dependencies to break.

@wbyoung
Copy link
Author

wbyoung commented Jan 17, 2017

No bug fix… just for matching consistency with Node.

You could incorporate this and bump to 0.2.0 as a semver "breaking change" for ^ ranges.

@addaleax
Copy link

Fwiw, process._tickDomainCallback doesn’t exist anymore in Node 9.3.0+ … I guess if you want a safe way to run the tick queue at a certain point, you can do a dummy MakeCallback from the native binding instead?

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.

3 participants