Add ThreadSafeCallback helper to schedule callbacks on the main thread #375

geovie · 2018-11-29T17:27:47Z

Hi,
this adds a little helper to call a JavaScript function from any rust thread as requested in #197.
~~I'm not sure if this will need a RFC, but I can write one if requested.~~
RFC

Feedback welcome, especially about the v8 scope setup.

Inspired by mika-fischer/napi-thread-safe-callback

maackle · 2018-12-18T20:36:12Z

I don't have the depth to comment on the viability of this, but I would love to have this functionality! I'm currently building something kind of complicated using Task because callbacks in threads are not possible.

amilajack · 2018-12-24T05:34:21Z

@geovie sorry for the delay on this. We'll review this soon

kjvalencik · 2019-01-31T15:26:12Z

Ignore my previous comment. I looked at the code and see that each is being run in a separate thread.

qm3ster · 2019-03-24T00:09:42Z

Oh wow, this is exactly what I need.
I've been trying to get "emitting" to work, even with help on the JS side, but I can neither call a callback asynchronously without terminating a Task, nor pass a tokio::sync::mpsc::Receiver on the "class" struct into a new Task because of lifetimes.

jrd-rocks · 2019-04-11T17:12:19Z

Thanks for this. I needed some sane way to get callbacks working. This has been a life saver!

x1957 · 2019-08-29T09:47:59Z

any updates?

dherman · 2019-08-29T17:29:18Z

@x1957 I'm discussing with @aep on Slack about updating this PR, and over on the RFC PR I've asked @geovie if I can help finish up the RFC. Would love to get this landed!

dignifiedquire · 2019-09-13T23:32:23Z

I tried the new branch, and it breaks unfortunately, using setTimeout + a loop in the thread, seems to results in the callback from setTimeout to never be called.

dignifiedquire · 2019-09-13T23:37:13Z

you can see the issue here: https://github.com/deltachat/deltachat-neon/blob/master/test.js#L14

dignifiedquire · 2019-09-14T09:59:29Z

sorry, it was my mistake, I was putting too much work into the cb.schedule closure, the PR in #429 does work nicely for me

crates/neon-runtime/native/src/neon.cc

geovie · 2019-09-15T20:23:18Z

src/eventhandler/mod.rs

+
+impl EventHandler {
+    pub fn new(callback: Handle<JsFunction>) -> Self {
+        TaskContext::with(|mut cx: TaskContext| {


Is TaskContext correct here?

I don't think it's correct, unfortunately. In fact, this is a mistake we made in the RFC--this API is impossible because it doesn't have access to the current context.

One option is to take the context as the first argument, but then this loses most of the convenience over the bind method.

I'm thinking the simplest thing would be:

Remove this method

Rename bind back to new

Defer the creation of convenience methods to the future. They could potentially be built-in methods of the Context trait, like cx.event_handler(func). But there's an open question in my mind about whether you want to offer conveniences for both Rust callbacks (e.g. cx.event_handler(|cx| { ... }) and JS callbacks (e.g. cx.event_handler(func)). It seems OK to wait for some usage experience before we know exactly what convenience shorthands we want.

@geovie @kjvalencik What do you think?

I don't think it's impossible, but the reference to the global context would need to be lazily created as part of the callback and not here.

With that said, I agree that it follows the existing patterns in neon better to add it to the Context trait.

I also support deferring the convenience method to a separate PR.

geovie · 2019-09-15T20:24:07Z

src/eventhandler/mod.rs

+
+    pub fn schedule<T, F>(&self, arg_cb: F)
+        where T: Value,
+              F: for<'a> FnOnce(&mut TaskContext<'a>) -> Vec<Handle<'a, T>>,


I'd love to have IntoIterator<...> instead of Vec<..>, but I couldn't get it to work with the lifetimes....

This could be a future improvement. I think to benefit from this, it would require a more holistic change to arguments in Neon.

geovie · 2019-09-15T20:27:53Z

src/eventhandler/mod.rs

+        let func: Handle<JsFunction> = Handle::new_internal(JsFunction::from_raw(func));
+        let callback: Box<F> = Box::from_raw(mem::transmute(callback));
+        let args = callback(&mut cx);
+        let _result = func.call(&mut cx, this, args);


Here the result (return value or exception) is discarded, I'm not sure if this is okay.
EventHandler::schedule_with was added to allow to use the result.

geovie · 2019-09-15T20:31:37Z

@dherman @kjvalencik I've updated the PR with the changes from the RFC

kjvalencik · 2019-09-26T14:18:18Z

@geovie Are you able to update this with the latest changes from the RFC? @aep also appears to be working on this. Can you coordinate? Thanks! I would ❤️ to get this merged.

geovie · 2019-09-26T14:22:57Z

@kjvalencik The PR is already up to date with the RFC, I will rebase it later. Any review is welcome

kjvalencik · 2019-09-26T14:31:29Z

@geovie Ah, sorry I missed that update! Thanks! I'll start reviewing.

src/eventhandler/mod.rs

kjvalencik · 2019-10-02T12:46:35Z

@geovie Unfortunately, files got moved around in #440 to prep for N-API. Are you able to resolve the conflicts? Let me know if you would like me to help.

Overall the changes look great and I'm ready to merge once the conflicts are resolved.

geovie · 2019-10-02T13:20:02Z

@kjvalencik rebased on master, should be good once CI passes

dherman

@geovie This is truly amazing work. Thank you so much! I do have a number of suggestions for improvement, as well as a couple of tweaks to the API we came up with in the RFC discussion. But I don't think it's too far away from being ready. Feel free to ping me on Slack to discuss!

crates/neon-runtime/src/nan/threadsafecb.rs

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

src/eventhandler/mod.rs

crates/neon-sys/native/src/neon_threadsafe_cb.h

dherman · 2019-10-14T18:57:06Z

src/eventhandler/mod.rs

+
+impl EventHandler {
+    pub fn new(callback: Handle<JsFunction>) -> Self {
+        TaskContext::with(|mut cx: TaskContext| {


I don't think it's correct, unfortunately. In fact, this is a mistake we made in the RFC--this API is impossible because it doesn't have access to the current context.

One option is to take the context as the first argument, but then this loses most of the convenience over the bind method.

I'm thinking the simplest thing would be:

Remove this method

Rename bind back to new

Defer the creation of convenience methods to the future. They could potentially be built-in methods of the Context trait, like cx.event_handler(func). But there's an open question in my mind about whether you want to offer conveniences for both Rust callbacks (e.g. cx.event_handler(|cx| { ... }) and JS callbacks (e.g. cx.event_handler(func)). It seems OK to wait for some usage experience before we know exactly what convenience shorthands we want.

@geovie @kjvalencik What do you think?

src/eventhandler/mod.rs

crates/neon-sys/native/src/neon_threadsafe_cb.h

src/eventhandler/mod.rs

bochaco · 2019-10-23T19:39:57Z

Thanks a lot @geovie , I guess I cannot get the value out of the cb.schedule_with call right? that's async and no way to wait for a result from it, unless perhaps using a mpsc channel? or what would you suggest for such an scenario?

kdy1 · 2019-11-28T11:38:51Z

When will be this feature merged?

Acanguven · 2019-12-05T14:17:29Z

Any updates on this? It will probably make Neon lot more useful and amazing. Because the current state of this solution limits us as we always have to use node as the data source.

dherman

We are so close! Thank you so much for keeping on this, @geovie. We have so many people who will benefit from this work.

We just have one last thing to change, in order to ensure we aren't creating something that we'll have to break compatibility on for the N-API transition. See the comment inline for details.

Thanks again!

dherman · 2019-12-13T18:13:02Z

crates/neon-sys/native/src/neon.cc

@@ -522,17 +522,23 @@ extern "C" void Neon_Task_Schedule(void *task, Neon_TaskPerformCallback perform,
  neon::queue_task(internal_task);
 }

-extern "C" void* Neon_ThreadSafeCallback_New(v8::Local<v8::Value> self, v8::Local<v8::Function> callback) {
+extern "C" void* Neon_EventHandler_New(v8::Local<v8::Function> callback) {


This was a nice idea, but unfortunately it's not future-proof for N-API, which doesn't have an equivalent of isolate::GetCurrent() and isolate->GetCurrentContext(). And in fact, this means the bind API is also not future-proof for N-API.

What this means is, we really need the EventHandler to take a Neon context as an explicit argument to its constructor(s).

So the best path forward is going to be: let's just drop this down to a single constructor, which takes three arguments: cx, self, and callback. It's a noticeably less convenient API, but I think that's OK to get started. Down the road, we can experiment with convenience methods of cx (just like the other convenience constructors such as cx.string(...) etc), like cx.event_handler(...).

But let's wait on the convenience forms and for now just have one method in the API:

EventHandler::new(cx, self, callback)

@dherman Should be good now, please take a final look! 😄

dherman · 2019-12-13T18:22:53Z

Any updates on this? It will probably make Neon lot more useful and amazing. Because the current state of this solution limits us as we always have to use node as the data source.

@Acanguven Thanks for keeping us on task (no pun intended) :P I've just done a review on the latest revisions. @geovie Hopefully the next round should be the last needed revision!

dherman

Looks perfect! Thanks for the amazing work, @geovie, I think this is ready to merge.

dherman · 2019-12-19T21:50:45Z

Ah, I see we're failing some Windows builds. I reran and it's failing again. Investigating...

Others · 2019-12-29T22:35:26Z

I was curious about this test failure. Here are the two interesting bits of the CI build that failed:
compile fail stuff
electron/gyp fail stuff

dherman · 2020-01-24T23:08:09Z

So the compile-fail bug is probably just the usual flakiness of those tests (they're extremely sensitive to tiny changes in the Rust compiler's output formatting). I'm going to need to just disable those tests until we can come up with a better strategy for them.

From searching around, I think the electron failure is also likely unrelated to this PR: it seems like this bug. I'm looking into what we can do to fix that issue.

dherman · 2020-01-27T22:54:19Z

OK I can confirm that the workaround from that issue (npm i -g npm@6.11.3) does indeed make the Electron tests pass.

This is a bit annoying! We'll need to track that node-gyp issue until they release a fix. For the time being, maybe we can get CI working by applying the workaround in the Appveyor config. I'll try a little PR to do that, which should rebase fine with this PR.

dherman · 2020-01-27T23:24:56Z

Testing out a workaround: #483

- add internal ThreadSafeCallback helper to schedule callbacks on the main thread

dherman · 2020-01-28T19:28:38Z

I got CI working again on master with #483 so I went ahead and rebased this PR -- let's see how rerunning CI goes! 🤞

dherman · 2020-01-28T19:45:19Z

Doh! It looks like the changes in master did break this PR (my fault, since it took me so long to diagnose these CI issues).

@geovie Would you like me to try to fix this in your PR? (GitHub gives me permission to update your PR, but I don't want to push non-rebase changes without your permission.) I'm happy to do it unless you'd rather.

geovie · 2020-01-28T20:48:15Z

@dherman Go for it, I won't manage to do it this week...

internal refactor.

dherman · 2020-01-29T22:27:25Z

Next step to get CI working should be to conditionally replace the implementation with unimplemented!() for the N-API backend for now. I'll do that next...

using the N-API backend, since that's not yet implemented.

it.

anymos · 2020-08-26T14:08:46Z

Looking forward to see that part going live

geovie mentioned this pull request Nov 30, 2018

RFC: EventHandler neon-bindings/rfcs#25

Merged

geovie mentioned this pull request Dec 17, 2018

Call JavaScript function from any thread neon-bindings/rfcs#26

Closed

kjvalencik mentioned this pull request Dec 27, 2018

Question: Confused by examples to call JS function from Rust #382

Closed

kjvalencik mentioned this pull request Feb 13, 2019

add documentation to event-emitter neon-bindings/examples#27

Closed

aep mentioned this pull request Aug 29, 2019

Add ThreadSafeCallback helper to schedule callbacks on the main thread #429

Closed

geovie force-pushed the feature/threadsafe_callback branch from 060f578 to c5ffa7a Compare September 15, 2019 20:19

geovie commented Sep 15, 2019

View reviewed changes

kjvalencik reviewed Sep 26, 2019

View reviewed changes

src/eventhandler/mod.rs Outdated Show resolved Hide resolved

kjvalencik reviewed Sep 26, 2019

View reviewed changes

src/eventhandler/mod.rs Outdated Show resolved Hide resolved

kjvalencik reviewed Sep 26, 2019

View reviewed changes

src/eventhandler/mod.rs Outdated Show resolved Hide resolved

geovie force-pushed the feature/threadsafe_callback branch from d166f1a to 49b544b Compare September 28, 2019 12:49

geovie force-pushed the feature/threadsafe_callback branch from 49b544b to 45d28f7 Compare October 2, 2019 13:18

dherman requested changes Oct 14, 2019

View reviewed changes

geovie commented Oct 16, 2019

View reviewed changes

src/eventhandler/mod.rs Outdated Show resolved Hide resolved

kdy1 mentioned this pull request Nov 28, 2019

Javascript plugin swc-project/swc#471

Closed

7 tasks

dherman requested changes Dec 13, 2019

View reviewed changes

geovie force-pushed the feature/threadsafe_callback branch from 30d0b2b to e62daa6 Compare December 14, 2019 12:59

kdy1 mentioned this pull request Dec 17, 2019

Something like @babel/preset-env swc-project/swc#104

Closed

1 task

dherman approved these changes Dec 19, 2019

View reviewed changes

geovie added 4 commits January 28, 2020 11:24

Add EventHandler to call JS function from secondary threads

85795c4

- add internal ThreadSafeCallback helper to schedule callbacks on the main thread

address MR comments

2782674

address MR comments

ed5483e

remove bind, add implicit cx argument to new

df135a5

dherman force-pushed the feature/threadsafe_callback branch from e62daa6 to df135a5 Compare January 28, 2020 19:24

Change cx.isolate() to cx.env(), based on @kjvalencik's recent

26ff7a5

internal refactor.

dherman added 2 commits January 29, 2020 17:32

Conditionally compile out the implementation of EventHandler::new when

c465be8

using the N-API backend, since that's not yet implemented.

Put the API behind a flag so we can merge it safely but still iterate on

4f3b1f2

it.

kjvalencik approved these changes Jan 31, 2020

View reviewed changes

dherman merged commit 03d31b6 into neon-bindings:master Jan 31, 2020

Acanguven mentioned this pull request Feb 5, 2020

Ability to call Javascript methods from Rust threads #197

Closed

Add ThreadSafeCallback helper to schedule callbacks on the main thread #375

Add ThreadSafeCallback helper to schedule callbacks on the main thread #375

Conversation

geovie commented Nov 29, 2018 • edited Loading

maackle commented Dec 18, 2018

amilajack commented Dec 24, 2018

kjvalencik commented Jan 31, 2019 • edited Loading

qm3ster commented Mar 24, 2019

jrd-rocks commented Apr 11, 2019

x1957 commented Aug 29, 2019

dherman commented Aug 29, 2019

dignifiedquire commented Sep 13, 2019

dignifiedquire commented Sep 13, 2019

dignifiedquire commented Sep 14, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geovie commented Sep 15, 2019

kjvalencik commented Sep 26, 2019

geovie commented Sep 26, 2019

kjvalencik commented Sep 26, 2019

kjvalencik commented Oct 2, 2019 • edited Loading

geovie commented Oct 2, 2019

dherman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bochaco commented Oct 23, 2019 • edited Loading

kdy1 commented Nov 28, 2019

Acanguven commented Dec 5, 2019 • edited Loading

dherman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dherman commented Dec 13, 2019

dherman left a comment

Choose a reason for hiding this comment

dherman commented Dec 19, 2019

Others commented Dec 29, 2019

dherman commented Jan 24, 2020

dherman commented Jan 27, 2020

dherman commented Jan 27, 2020

dherman commented Jan 28, 2020

dherman commented Jan 28, 2020

geovie commented Jan 28, 2020

dherman commented Jan 29, 2020

anymos commented Aug 26, 2020

geovie commented Nov 29, 2018 •

edited

Loading

kjvalencik commented Jan 31, 2019 •

edited

Loading

kjvalencik commented Oct 2, 2019 •

edited

Loading

bochaco commented Oct 23, 2019 •

edited

Loading

Acanguven commented Dec 5, 2019 •

edited

Loading