Skip to content

Worker code needs to be adapted to canary changes #66

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
targos opened this issue Jun 9, 2018 · 4 comments
Closed

Worker code needs to be adapted to canary changes #66

targos opened this issue Jun 9, 2018 · 4 comments

Comments

@targos
Copy link
Member

targos commented Jun 9, 2018

After changes to the V8 API and commits df12c1d and 508941f, the new worker code fails to compile.

/cc @addaleax

@addaleax
Copy link
Member

addaleax commented Jun 9, 2018

This would be a fix for the compilation failures; there are test failures that look more complicated:

[See below for updated diff]

It might be worth noting that 508941f, in this form, is semver-major, i.e. we can’t backport it to v10.x like this – not sure whether that’s an issue or not?

@addaleax
Copy link
Member

Okay, this works as a full patch for the issues:

diff --git a/src/node_worker.cc b/src/node_worker.cc
index 86d9ed4d6a93..b4e55a360f8d 100644
--- a/src/node_worker.cc
+++ b/src/node_worker.cc
@@ -62,9 +62,9 @@ Worker::Worker(Environment* env, Local<Object> wrap)
 
   array_buffer_allocator_.reset(CreateArrayBufferAllocator());
 
-  isolate_ = NewIsolate(array_buffer_allocator_.get());
-  CHECK_NE(isolate_, nullptr);
   CHECK_EQ(uv_loop_init(&loop_), 0);
+  isolate_ = NewIsolate(array_buffer_allocator_.get(), &loop_);
+  CHECK_NE(isolate_, nullptr);
 
   thread_exit_async_.reset(new uv_async_t);
   thread_exit_async_->data = this;
@@ -240,6 +240,7 @@ void Worker::DisposeIsolate() {
   platform->CancelPendingDelayedTasks(isolate_);
 
   isolate_data_.reset();
+  platform->UnregisterIsolate(isolate_);
 
   isolate_->Dispose();
   isolate_ = nullptr;

@targos
Copy link
Member Author

targos commented Jun 10, 2018

@addaleax thanks!

It might be worth noting that 508941f, in this form, is semver-major, i.e. we can’t backport it to v10.x like this – not sure whether that’s an issue or not?

That's not an immediate issue, as this won't land before V8 6.9. We are currently not planning to pull V8 6.9 into v10.x AFAIK.

@targos
Copy link
Member Author

targos commented Jun 10, 2018

Incorporated in nodejs/node@042988a

Last update is successful: https://ci.nodejs.org/view/MyJobs/job/node-update-v8-canary/487/ 🎉

@targos targos closed this as completed Jun 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants