Handle early finalization of TSFN by Node.js by indutny · Pull Request #744 · neon-bindings/neon

indutny · 2021-05-20T18:32:09Z

Thread-safe functions can get released early by Node.js itself during
the teardown of the Environment that holds them. When this happens -
finalize_cb of the thread-safe function is invoked to notify us. This
change makes us remember if the thread-safe function was finalized
before we got into the Drop implementation for it so that we don't
deallocate it twice.

Additionally, we should not invoke call_threadsafe_function after
either finalize_cb call or napi_closing return value from previous
call_threadsafe_function. Handle this by holding a mutex around the
call, which doesn't introduce any additional locking because
call_threadsafe_function itself holds another mutex while active.

See: #739 (comment)

kjvalencik · 2021-05-20T18:42:34Z

crates/neon-runtime/src/napi/tsfn.rs

 /// function for scheduling tasks to execute on a JavaScript thread.
 pub struct ThreadsafeFunction<T> {
    tsfn: Tsfn,
+    is_finalized: Arc<Mutex<bool>>,


Can this be an AtomicBool instead of a Mutex<bool>? I'm also not clear why it needs to be an Arc.

Arc provides a nice way of passing the pointer to the C function, because unlike Box you can clone it before turning into a raw pointer, and then decrement the reference count when turning it back from the raw pointer to the Arc instance and dropping it.

I think it could be AtomicBool. Let me try that in a moment.

Moved to AtomicBool. @jrose-signal must be really happy to see this happen 😉

Yeah, it has to be something indirect because ThreadsafeFunction is passed around by value. Box is probably safe because the finalizer callback is guaranteed to be invoked before Drop::drop completes (either Node invokes it on shutdown or we invoke it in drop), but it's a lot easier to mess that up somehow.

jrose-signal · 2021-05-20T19:24:39Z

I think you're right about the race in #739 (comment). :-( A 99% multithreading fix can be worse than no fix, because it makes it harder to track down further issues.

indutny-signal · 2021-05-20T19:30:35Z

There more I think about it the more I agree that this is a serious bug in Node.js that has to be fixed. Another race condition that I just discovered is with the finalize_cb. It is invoked after closing all UV handles that are used in the thread-safe function so Node will just crash if you try to release it at a bad time.

indutny · 2021-05-22T18:53:45Z

Nevermind, I'm a fool. ThreadsafeFunction can only be destroyed from the main loop because it might hold a reference to a JS function. The documentation explicitly says:

Aside from the main loop thread, no threads should be using the thread-safe function after the finalize callback completes.

Now, I've checked all code paths and it cannot crash as I previously thought. To prevent the race condition in this patch, however, we'll have to hold the mutex for the duration of the drop() method. I'll modify this PR accordingly.

indutny · 2021-05-22T19:04:07Z

Alright, pushed an update. Sadly AtomicBool had to go. When finalize() and drop() are called concurrently - two different orderings are possible:

finalize() gets to the mutex first (and returns) and thus drop() will see is_finalized = true and won't call release
drop() holds the mutex and observes is_finalized = false, finalize() is waiting on mutex on the UV loop thread and does not return until drop() calls the release(). In this case release() will attempt to send a uv_async_t event to the UV loop, which won't ever trigger because right after finalize() returns Node.js will close the uv_async_t handle

In both of the cases we get the desired result. This PR is ready for a review again.

indutny · 2021-05-22T19:37:13Z

Had to add a mutex to the call() method as well because we shouldn't be calling it after either of:

getting napi_closed from a previous call
finalize_cb was called

This is quite inconvenient for Neon, but would likely be needed until we'll get some resolution in nodejs/node#38775 (which would be an documentation change, but hopefully not ABI change)

Thread-safe functions can get released early by Node.js itself during the teardown of the Environment that holds them. When this happens - `finalize_cb` of the thread-safe function is invoked to notify us. This change makes us remember if the thread-safe function was finalized before we got into the `Drop` implementation for it so that we don't deallocate it twice. Additionally, we should not invoke `call_threadsafe_function` after either `finalize_cb` call or `napi_closing` return value from previous `call_threadsafe_function`. Handle this by holding a mutex around the call, which doesn't introduce any additional locking because `call_threadsafe_function` itself holds another mutex while active.

indutny · 2021-05-22T23:01:19Z

Force pushed PR with a rebase over main branch and all changes squashed.

jrose-signal · 2021-05-24T17:17:11Z

crates/neon-runtime/src/napi/tsfn.rs

+        // Node.js
+        if *is_finalized {
+            return;
+        }


Don't you need to drop the lock before calling napi::release_threadsafe_function?

Nope, actually I don't the idea here is that we want finalize_cb to block until release_threadsafe_function completes, because otherwise finalize_cb might finish first and delete the tsfn in C++.

Mutex isn't re-entrant, though, so in the non-early-exit case you'll lock, call release_threadsafe_function, and fail when it's time to lock again.

release_threadsafe_function doesn't call finalize_cb directly. It schedules an event on the loop thread and executes finalize_cb in another tick.

indutny-signal · 2021-05-25T16:27:40Z

See: napi-rs/napi-rs#522 . We are not the first to run into it! napi-rs took a milder approach of just leaking the function and letting Node.js reclaim it 😂

kjvalencik · 2021-06-01T20:49:19Z

After fixing a leak in #750, this started happening every time with the global queue.

I think I fully understand this fix now and appreciate it. Thanks!

indutny-signal · 2021-06-01T20:52:15Z

No worries! Let's land the thing! 😉

kjvalencik · 2021-06-01T21:02:05Z

Thanks so much for this fix! I'm going to release it along with this PR: #750

indutny mentioned this pull request May 20, 2021

Reuse ThreadsafeFunction in EventQueue #739

Merged

kjvalencik reviewed May 20, 2021

View reviewed changes

indutny mentioned this pull request May 22, 2021

node-api ThreadsafeFunction is a leaky abstraction nodejs/node#38775

Open

indutny force-pushed the fix/tsfn-crash branch from e97bbd0 to b5a672e Compare May 22, 2021 23:00

jrose-signal reviewed May 24, 2021

View reviewed changes

kjvalencik merged commit dd0af64 into neon-bindings:main Jun 1, 2021

indutny-signal deleted the fix/tsfn-crash branch June 1, 2021 21:04

This was referenced Jun 6, 2021

Update dependency neon-cli to ^0.10.0 amilajack/os-type#7

Open

Update dependency neon-cli to ^0.10.0 amilajack/neon-benchmarks#56

Open

Update dependency neon-cli to ^0.10.1 sqlite-js/sqlite#57

Open

renovate bot mentioned this pull request Jul 1, 2021

fix(deps): update dependency neon-cli to ^0.8.3 amilajack/node-disk-utility#71

Merged

1 task

SevInf mentioned this pull request Jul 5, 2022

Memory and safety issues with ThreadsafeFunction napi-rs/napi-rs#1220

Open

Comments

Conversation

indutny commented May 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kjvalencik May 20, 2021

Choose a reason for hiding this comment

Uh oh!

indutny-signal May 20, 2021

Choose a reason for hiding this comment

Uh oh!

indutny-signal May 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jrose-signal May 20, 2021

Choose a reason for hiding this comment

Uh oh!

jrose-signal commented May 20, 2021

Uh oh!

indutny-signal commented May 20, 2021

Uh oh!

indutny commented May 22, 2021

Uh oh!

indutny commented May 22, 2021

Uh oh!

indutny commented May 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

indutny commented May 22, 2021

Uh oh!

jrose-signal May 24, 2021

Choose a reason for hiding this comment

Uh oh!

indutny-signal May 24, 2021

Choose a reason for hiding this comment

Uh oh!

jrose-signal May 24, 2021

Choose a reason for hiding this comment

Uh oh!

indutny-signal May 24, 2021

Choose a reason for hiding this comment

Uh oh!

indutny-signal commented May 25, 2021

Uh oh!

kjvalencik commented Jun 1, 2021

Uh oh!

indutny-signal commented Jun 1, 2021

Uh oh!

kjvalencik commented Jun 1, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

indutny commented May 20, 2021 •

edited

Loading

indutny-signal May 20, 2021 •

edited

Loading

indutny commented May 22, 2021 •

edited

Loading