Skip to content

fix: enable worker threads in ELECTRON_RUN_AS_NODE#20416

Merged
codebytere merged 1 commit intomasterfrom
worker-threads-should-work
Oct 7, 2019
Merged

fix: enable worker threads in ELECTRON_RUN_AS_NODE#20416
codebytere merged 1 commit intomasterfrom
worker-threads-should-work

Conversation

@codebytere
Copy link
Copy Markdown
Member

@codebytere codebytere commented Oct 3, 2019 •
edited
Loading

Description of Change

Fix Node.js' worker_threads module when Electron is run with ELECTRON_RUN_AS_NODE. The fundamental issue here is that Electron sets NODE_USE_V8_PLATFORM to false, because Electron initializes the v8 platform itself (in javascript_environment.cc), and Node.js worker thread initialization relies on the platform it uses having been set inside code guarded by NODE_USE_V8_PLATFORM.

This commit fixes this problem by changing node_worker.cc to use the three-arg
implementation of NewIsolate to prevent it trying to use a possibly-null ptr.
cc @ckerr @nornagon @zcbenz

Checklist

Release Notes

Notes: Fixed Node.js' worker_threads in ELECTRON_RUN_AS_NODE.

@codebytere codebytere requested a review from a team as a code owner October 3, 2019 19:41
@electron-cation electron-cation Bot added the new-pr 🌱 PR opened recently label Oct 3, 2019
@codebytere codebytere force-pushed the worker-threads-should-work branch 4 times, most recently from 4bdb755 to 19fc67b Compare October 3, 2019 20:03
@codebytere codebytere changed the title [wip] fix: enable worker threads in ELECTRON_RUN_AS_NODE fix: enable worker threads in ELECTRON_RUN_AS_NODE Oct 3, 2019
@codebytere codebytere force-pushed the worker-threads-should-work branch from 19fc67b to b784d0a Compare October 3, 2019 20:06
@durasj durasj mentioned this pull request Oct 3, 2019
Closed
deepak1556
deepak1556 reviewed Oct 3, 2019
View reviewed changes
Comment thread patches/node/fix_enable_worker_threads.patch Outdated
@codebytere codebytere force-pushed the worker-threads-should-work branch from 8161daf to ed486ed Compare October 4, 2019 08:26
@jkleinsc jkleinsc requested a review from deepak1556 October 4, 2019 14:28
deepak1556
deepak1556 approved these changes Oct 4, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@deepak1556 deepak1556 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@electron-cation electron-cation Bot removed the new-pr 🌱 PR opened recently label Oct 4, 2019
@codebytere codebytere mentioned this pull request Oct 5, 2019
Closed
2 tasks
@codebytere codebytere force-pushed the worker-threads-should-work branch from ed486ed to 8649c8c Compare October 6, 2019 09:07
@codebytere codebytere force-pushed the worker-threads-should-work branch from 8649c8c to 77198f8 Compare October 6, 2019 09:09
@Inomezi
Copy link
Copy Markdown

Inomezi commented Oct 6, 2019

Hi, is it working now ? I'm Using electron 6.0.11 , but still got error The V8 platform used by this instance of Node does not support creating Workers

@codebytere codebytere merged commit 9b534e9 into master Oct 7, 2019
@release-clerk
Copy link
Copy Markdown

release-clerk Bot commented Oct 7, 2019

Release Notes Persisted

Fixed Node.js' worker_threads in ELECTRON_RUN_AS_NODE.

@codebytere codebytere deleted the worker-threads-should-work branch October 7, 2019 09:30
@trop
Copy link
Copy Markdown
Contributor

trop Bot commented Oct 7, 2019

I was unable to backport this PR to "7-0-x" cleanly;
you will need to perform this backport manually.

@codebytere codebytere mentioned this pull request Oct 7, 2019
Merged
@trop
Copy link
Copy Markdown
Contributor

trop Bot commented Oct 7, 2019

@codebytere has manually backported this PR to "7-0-x", please check out #20456

@codebytere codebytere mentioned this pull request Oct 7, 2019
Merged
@trop
Copy link
Copy Markdown
Contributor

trop Bot commented Oct 7, 2019

@codebytere has manually backported this PR to "6-0-x", please check out #20457

@Inomezi
Copy link
Copy Markdown

Inomezi commented Oct 10, 2019

Hello, it still isn't work ?

@codebytere
Copy link
Copy Markdown
Member Author

codebytere commented Oct 10, 2019

@Inomezi what released version did you test with?

@Inomezi
Copy link
Copy Markdown

Inomezi commented Oct 10, 2019 •
edited
Loading

@Inomezi what released version did you test with?

I tested it on version 6.0.12 and 7.0.0-beta.6. When i calling new Worker("path") i getting error: Uncaught Error: The V8 platform used by this instance of Node does not support creating Workers.
What's version i need to use ?

@codebytere
Copy link
Copy Markdown
Member Author

codebytere commented Oct 10, 2019

@Inomezi that's a different issue; i'm still working on that! It's not fixed yet.

@Inomezi
Copy link
Copy Markdown

Inomezi commented Oct 10, 2019

@Inomezi that's a different issue; i'm still working on that! It's not fixed yet.

Okay, i'll wait. There is no version on which I could create worker_threads now?

@laverdet laverdet mentioned this pull request Oct 28, 2019
Closed
@addaleax
Copy link
Copy Markdown
Contributor

addaleax commented Oct 29, 2019

@Inomezi that's a different issue; i'm still working on that! It's not fixed yet.

@codebytere Was that something we discussed in the last weeks? Is that anything that I could help with?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@deepak1556 deepak1556 deepak1556 approved these changes

@ckerr ckerr Awaiting requested review from ckerr

@nornagon nornagon Awaiting requested review from nornagon

@zcbenz zcbenz Awaiting requested review from zcbenz

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@codebytere @Inomezi @addaleax @deepak1556 @sofianguy