deasync icon indicating copy to clipboard operation
deasync copied to clipboard

Match tick callback to Node's logic

Open wbyoung opened this issue 8 years ago • 3 comments

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).

wbyoung avatar Jan 16 '17 20:01 wbyoung

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.

abbr avatar Jan 17 '17 02:01 abbr

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.

wbyoung avatar Jan 17 '17 05:01 wbyoung

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?

addaleax avatar Dec 14 '17 12:12 addaleax