Handle early finalization of TSFN by Node.js

[indutny]

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]

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

[indutny-signal]

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.

[indutny-signal]

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

[jrose-signal]

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]

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]

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]

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]

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]

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)

[indutny]

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

[jrose-signal]

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

[indutny-signal]

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

[jrose-signal]

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.

[indutny-signal]

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]

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]

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]

No worries! Let's land the thing! 😉

[kjvalencik]

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