Task API for running background tasks in the libuv thread pool #214
Conversation
crates/neon-runtime/src/neon_task.h
Outdated
| } | ||
|
|
||
| void complete() { | ||
| Nan::HandleScope scope; |
There was a problem hiding this comment.
You should enter an Isolate::Scope and Context::Scope here:
v8::Isolate::Scope isolate_scope(isolate_);
v8::HandleScope handle_scope(isolate_); // Or Nan::HandleScope
v8::Local<v8::Context> context = v8::Local<v8::Context>::New(isolate_, context_);
v8::Context::Scope context_scope(context);
// ...Store the context when the task is created because it can change.
Using v8::Isolate::GetCurrentContext() in this method is not sound because it's possible no context has been entered or it's a different one from the one that created the task.
crates/neon-runtime/src/neon_task.h
Outdated
| } | ||
|
|
||
| v8::Local<v8::Function> callback = v8::Local<v8::Function>::New(isolate_, callback_); | ||
| callback->Call(isolate_->GetCurrentContext(), v8::Null(isolate_), 2, argv); |
There was a problem hiding this comment.
If try_catch.HasCaught() is true, there is still an exception pending here and you can't call into the VM. If you scope the TryCatch in curly braces, it will have been cleared.
Whether it's a good idea to catch exceptions and continue is up for debate, though.
crates/neon-runtime/src/neon_task.h
Outdated
| } | ||
|
|
||
| ~Task() { | ||
| callback_.Reset(); |
There was a problem hiding this comment.
Not strictly necessary, ~Persistent() does that by default.
crates/neon-runtime/src/neon_task.h
Outdated
| }; | ||
|
|
||
| void execute_task(uv_work_t *request) { | ||
| Task *task = static_cast<Task*>(request->data); |
There was a problem hiding this comment.
FWIW, we use a container_of-like macro for that in libuv and node.js. Saves one level of pointer indirection.
|
Nice! When this lands... we'll really be cooking with 🔥. A couple things that crossed my mind:
|
|
@bnoordhuis @jedireza Thank you both so much! I'll make sure to incorporate all your feedback before merging. |
- eliminate redundant resetting of `Persistent` members in destructor - save the context (ie realm) at the time the task is created - create an `Isolate::Scope` and `Context::Scope` on the stack before calling the callback - scope the `TryCatch` so it clears the pending exception before calling the callback
| argv[1] = v8::Undefined(isolate_); | ||
|
|
||
| { | ||
| v8::TryCatch trycatch(isolate_); |
There was a problem hiding this comment.
Is it necessary to always use try catch?
It can cause a big performance hit with some callback since V8 cannot jit complile the functions in the try catch stack. https://news.ycombinator.com/item?id=3797822
Perhaps a flag could be passed so there is a way to control this?
There was a problem hiding this comment.
Please refer to my note about try catch performance.
Would love to hear your thoughts about this issue
There was a problem hiding this comment.
try/catch no longer deoptimizes in recent versions of V8. But even if it did, it wouldn't matter because this is C++ code, not JS code - the performance pitfall only applied to JS functions with a try/catch statement.
There was a problem hiding this comment.
Sorry my bad,didn't notice the try catch was only before the callback, thanks for the answer. Anything I could do to help get this pull request accepted asap?
I really need async support and would gladly contribute if needed
There was a problem hiding this comment.
Sorry my bad,didn't notice the try catch was only before the callback, thanks for the answer. Anything I could do to help get this pull request accepted asap?
I really need async support and would gladly contribute if needed
|
Thanks to a bunch of debugging, we should be back in a state where this can build. Should be able to merge soon! I'm re-running the CI builds now to see what's left to fix. |
|
AppVeyor is being very suspiciously flakey lately but I'm pretty confident the builds hanging are AppVeyor's fault since we haven't seen them happen at all on multiple machines. And there were no runtime test failures. So once I reran the CI build a few times and got all green I felt confident enough to merge. I'll reach out to AppVeyor to see what's going wrong. |
No description provided.