Open
Conversation
Owner
|
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. |
Author
|
No bug fix… just for matching consistency with Node. You could incorporate this and bump to |
Closed
|
Fwiw, |
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
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.
Node calls the tick callback from two places:
MakeCallbackandAsyncWrap::MakeCallback.Originally, I thought maybe
MakeCallbackcould be used withindeasync.ccto gain the same behavior with purely public APIs. My goal was to remove the the use of the private_tick<?Domain>Callback, but invocation oftick_callback_functionis actually blocked because node will always be in a nestedAsyncCallbackScopefor bothMakeCallbackandAsyncWrap::MakeCallback. Initial code execution begins in such a context, and all future callbacks are done via subclasses ofAsyncWrap, so theirMakeCallbackinvocation always puts them back in anAsyncCallbackScope.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
domainis required it sets up domain use which changes thetick_callback_functionin theEnvironmentand also adds adomainproperty toprocess. So at the very least, I think it makes sense to invoke the callback that Node would be invoking once no longer in a nestedAsyncCallbackScope.While this doesn't really change much today since
_tickCallbackand_tickDomainCallbackfunctions are nearly identical (and the domain version is graceful with the absence of adomain), it does better future-proof the code a little (especially since domains are deprecated).