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

fix(ext/ffi): Fix re-ref'ing UnsafeCallback #17704

Merged
merged 8 commits into from
Feb 22, 2023

Conversation

aapoalas
Copy link
Collaborator

@aapoalas aapoalas commented Feb 9, 2023

Currently, UnsafeCallback ref'ing and unref'ing uses a CancelHandle in the UnsafeCallback resource. When an UnsafeCallback gets unref'ed (ref counter goes back to 0), its CancelHandle is triggered to resolve the Future / Promise that keeps the event loop alive and provides a Waker for the CallbackInfo. If the same UnsafeCallback then gets re-ref'ed afterwards the same CancelHandle gets reused and it immediately cancels the Future, leading to Deno's event loop not staying alive and worst of all severing the Waker's tie to the Deno event loop.

There issue is that currently, an UnsafeCallback that is unref'ed (or never ref'ed in the first place) cannot wake the Deno event loop up as it either doesn't have a Waker, or the Waker is no longer tied to the event loop. So eg. this will not work:

const callback = new Deno.UnsafeCallback(...);
lib.symbols.register_callback(callback.pointer); // expect this to be called in a thread-safe manner at some point randomly
setTimeout(() => {}, 100000000000000000)

The timeout finishes in something like a 300 000 years but the callback will never be resolved: Any call to this callback will send Deno's event loop work to do, but the event loop can never be woken up.

Fix

  • Added UnsafeCallback.threadSafe() static constructor function that creates a callback and refs it once.
  • When a callback is ref'ed, it creates a Promise/Future with a Waker given to the C callback info that can be used to wake up the event loop. The Future is only resolved by cancelling when the callback resource is closed.
  • When a callback is unref'ed, the Promise is unref'ed using the core.unrefOp mechanism. ie. The Promise/Future does not get resolved or canceled, it is simply unref'ed.

@aapoalas
Copy link
Collaborator Author

aapoalas commented Feb 9, 2023

I added a static UnsafeCallback.threadSafe(definition, callback) constructor function. It internally just creates a new callback and immediately ref's it to make it thread-safe wakeable. With proper documentation this should, I think, be enough to not confuse users too badly.

@aapoalas aapoalas force-pushed the fix/ext-ffi/double-ref-callback branch from 1ae27bd to f544b51 Compare February 18, 2023 09:24
Copy link
Member

@littledivy littledivy left a comment

Choose a reason for hiding this comment

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

LGTM

@aapoalas aapoalas enabled auto-merge (squash) February 22, 2023 18:45
@aapoalas aapoalas merged commit 0f9daae into denoland:main Feb 22, 2023
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.

2 participants