feat(ext/ffi): Thread safe callbacks

[aapoalas]

Fairly okay working solution to thread safe FFI callbacks, complete with tests!

A new FfiState is saved into the OpState, containing the sender and receiver for a std::mpsc::sync_channel which is used for message passing between threads. Isolate threads (main and worker) are now identified using a thread-local Isolate pointer which gets assigned to the thread on UnsafeCallback creation. This allows an incoming callback to not only figure out if the thread it is being called on is an isolate thread but to also tell apart its own isolate thread from others, thus making it possible for workers to register and trigger callbacks originating in any thread while still ensuring that those callbacks are called on the appropriate isolate thread.

If the callback is found to be coming from a foreign thread (foreign isolate thread or completely foreign), it will create a new response SyncChannel and sends a message using its own copy of the sender side of the main SyncChannel. The message is a boxed closure which will call the actual deno_ffi_callback internal implementation and then send a message into the newly created response SyncChannel. The foreign thread will then block on the response SyncChannel until it receives the response message after which it unblocks the thread and deallocates the response channel and returns control to the caller of the callback.

Open questions:

• Channel type decided in the favour of an UnboundedChannel
• Isolate pointer comparison deemed apt, or at least not objected to
• Response sync_channel bound set to 0, making it a rendezvous channel
• Error handling for message passing?

[bartlomieju]

I believe dereferenced is misleading in this context

[aapoalas]

Any suggestion what might be a better term? I how Node documents their ref and unref functions and, surprise surprise, they use the verbs ref and unref and term ref'd (or maybe ref'ed).

Should I just align with that?

[aapoalas]

Used ref'ed and unref'ed for now.

[bartlomieju]

Maybe add a check that we've replaced a null pointer here?

[aapoalas]

Added a check to only replace the null pointer once.

[aapoalas]

One question that I thought of and tested that locally this caused no issues is the response sync_channel. Currently I create it with a bound of 1, meaning that if somehow theoretically the isolate thread manages to call the do_ffi_callback and then send a response before the "async" thread calls response_receiver.recv().unwrap() then the response can be buffered.

This seems unnecessary, as the sync_channel receiver side is literally this:

async_work_sender.unbounded_send(fut).unwrap(); // Send the message
response_receiver.recv().unwrap(); // Block on a response

The isolate thread would need to be lightning fast to poll the async work channel and do the work, and the calling thread would need to be absolutely massively busy to not get to call .recv() before the response sent. This does not seem to be a particularly realistic case.

As such the bound could perhaps be lowered to 0, making the channel a "rendezvous channel" which brings with it some perf optimisations I believe.

Opinions?

EDIT: I indeed changed this to a rendezvous channel with blocking on both ends for the other side to become available. This means that even if the receiving thread is very fast and the sending thread is very busy, they'll wait for each other on the channel and pass the response directly without using the buffer.

[littledivy]

LGTM. This introduces an event loop middleware - let's watch for any unrelated perf regressions.

[aapoalas]

LGTM. This introduces an event loop middleware - let's watch for any unrelated perf regressions.

Good point. The vector allocation might cause perf regression.