feat(core): Deno.core.setNextTickCallback #12771
Conversation
| scope.perform_microtask_checkpoint(); | ||
| } | ||
|
|
||
| // TODO(bartlomieju): Node also checks for absence of "rejection_to_warn" |
There was a problem hiding this comment.
Currently there's no handling of "unhandledRejection" in Deno so this conditional is skipped, I'm sure it will be problematic when trying to enable more of Node tests
There was a problem hiding this comment.
Side effect is that process.nextTick() doesn't behave properly if error is thrown during its callback
|
I verified that this PR allows us to polyfill |
| fn has_tick_scheduled( | ||
| scope: &mut v8::HandleScope, | ||
| _args: v8::FunctionCallbackArguments, | ||
| mut rv: v8::ReturnValue, | ||
| ) { | ||
| let state_rc = JsRuntime::state(scope); | ||
| let state = state_rc.borrow(); | ||
| rv.set(to_v8(scope, state.has_tick_scheduled).unwrap()); | ||
| } |
There was a problem hiding this comment.
Node does this by means of an ArrayBuffer, to avoid (slow) JS -> C++ calls. Not a blocker, merely an observation.
There was a problem hiding this comment.
Agreed, let's address in a follow up
There was a problem hiding this comment.
Actually I'm confused here - it seems to me that it's done by having a TickInfo bound from C++ to JS. Can you clarify what you meant?
There was a problem hiding this comment.
Node applies a trick where it makes a C struct available to JS land by using the struct's memory as a backing store for a typed array. That way C++ and JS land can communicate without expensive JS <-> C++ calls, they just read and write the struct's fields.
There was a problem hiding this comment.
I see, I'll be happy to explore this path, let's discuss it offline in details and I'll raise another PR
|
|
||
| let js_macrotask_cb_handles = state.borrow().js_macrotask_cbs.clone(); |
There was a problem hiding this comment.
This also clones all the v8::Global handles in the vec? That's a fairly expensive operation.
Also, I think I'm missing where js_macrotask_cbs is emptied. It looks like currently it only grows, never shrinks.
There was a problem hiding this comment.
Yeah I believe it does clone global handles, in practice there should be no more than two of them, but I can try not to clone them at all (however it's risky because I'd have to hold onto borrow of state and there's nothing preventing callback from using state as well, ergo we might run into BorrowError).
Also, I think I'm missing where js_macrotask_cbs is emptied. It looks like currently it only grows, never shrinks.
Correct, but until this PR there was only a single macrotask callback allowed (used by timers), this PR changes it to allow multiple callbacks. In practice there will be one callback for timers, one for process.nextTick, there might be more of them if users pull multiple versions of deno_std (and thus register multiple process.nextTick callback.
| return Ok(()); | ||
| } | ||
|
|
||
| let js_nexttick_cb_handles = state.borrow().js_nexttick_cbs.clone(); |
There was a problem hiding this comment.
Non-blocking observation: state.borrow() gets called a lot. It's more efficient to borrow once and drop when necessary.
It's possible it doesn't matter in practice because the compiler "sees through" and elides borrow() calls.
There was a problem hiding this comment.
Please see comment above about risk of running into BorrowError
Co-authored-by: Ben Noordhuis <[email protected]>
See denoland#12771 and denoland#12814. Needed to reland denoland#12908, which was reverted in denoland#12929.
This commit adds several new "Deno.core" bindings:
Additionally it changes "Deno.core.setMacrotaskCallback" to
allow registering multiple callbacks. All these changes were necessary
to polyfill "process.nextTick" in Node compat layer.
Closes #12735