[WIP]: src: implement NodeThreadScheduler

[DaAitch]

This is a WIP PR.

Did some verbose commenting in the code as I was fixing some potential bugs/leaks from DaAitch/napi-experimental@412a466 as napi_tsfn_abort mode was used.

This code should work, but I think we can do better.

I have a new idea of how we can get rid of heap allocation of NodeThreadScheduler, having it copyable, movable on the stack, thus feels more lightweight.
I'll work on it soon.

[DaAitch]

So I thought about, why not implementing it in the napi itself like this nodejs commit?
It's a more lightweight approach. I did some implementation, but there is a bug and I cannot find it, see failing tests. @gabrielschulhof I had a look at your threadsafe_function impl and tried to do it same way. Do you think you can have a look at it? Really appreciate.

[gabrielschulhof]

@DaAitch AFAICT napi_threadsafe_callback doesn't use a queue like napi_threadsafe_function does, and uv_async_t does not guarantee that for every uv_async_send from a secondary thread it will wake up the main thread.

OTOH it should be possible to break v8impl::ThreadSafeFunction into a base class of which v8impl::ThreadSafeFunction would be a subclass, and then create a second subclass which has the ThreadsafeCallback semantics.

TBH though the fact that we wish for this to be a single instance to be used by the entire addon for any number of reasons is beginning to turn this into a generic asynchronous queue implementation – which is perhaps what we really want, and which is perhaps what the base class will become.

[DaAitch]

@DaAitch AFAICT napi_threadsafe_callback doesn't use a queue like napi_threadsafe_function does, and uv_async_t does not guarantee that for every uv_async_send from a secondary thread it will wake up the main thread.

Okay, I thought according to the doc, when I uv_async_init a handle, multiple uv_async_send of that handle in the same loop iteration may end up in only one callback, but per handle, and every callback has it's own handle. So if you say, it may not wake up the main loop, it means, that after a uv_async_send nothing may happen?

OTOH it should be possible to break v8impl::ThreadSafeFunction into a base class of which v8impl::ThreadSafeFunction would be a subclass, and then create a second subclass which has the ThreadsafeCallback semantics.

Okay yeah, I'm open to change impl details. I tried to just do it, get some feedback about maybe as threadsafe_function can do the same job, we maybe don't need it.

TBH though the fact that we wish for this to be a single instance to be used by the entire addon for any number of reasons is beginning to turn this into a generic asynchronous queue implementation – which is perhaps what we really want, and which is perhaps what the base class will become.

That's exactly where I started at 😆 , I tried a naive way and make setImmediate or nextProcess (can't remember) threadsafe, but for some reason it didn't work.

So @gabrielschulhof what are your feelings about working on this? I'm happy to have threadsafe_function to at least be able to call into nodejs from 2nd thread. This a feature, very basic, but not super important for everyone.

[DuBistKomisch]

I assume this is for #312? Looking forward to it!

[KevinEady]

Hi @DaAitch ,

I think this can be closed since ThreadSafeFunction has been added to the core.

cc: @mhdawson

[mhdawson]

Discussed in the N-API team meeting today. Given that ThreadSafeFunction, function callback is optional and ThreadSafeFunction is available in all current LTS lines we believe we can close this.

Closing let us know if that is not the right thing to do.