★ wanayoo — archive 1999 https://github.com/nodejs/node/pull/26497Nouvelle recherche | Portail wanayoo
Skip to content

/node

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

worker: implement worker.moveMessagePortToContext() #26497

Closed
wants to merge 3 commits into from

Conversation

Reviewers

@joyeecheung joyeecheung

@jasnell jasnell

@gengjiawen gengjiawen

Assignees
No one assigned
Labels
author ready lib / src semver-minor vm worker
Projects
None yet
Milestone
No milestone
7 participants
@addaleax @nodejs-github-bot @BridgeAR @chjj @jasnell @gengjiawen @joyeecheung
@addaleax
Copy link
Member

addaleax commented Mar 7, 2019

src,lib: allow running multiple per-context files

Create an lib/internal/per_context/ directory that can
host multiple files which we execute for each context.

src,lib: make DOMException available in all Contexts

This allows using DOMException from Node.js code for any
vm.Context.

worker: implement worker.moveMessagePortToContext()

This enables using MessagePorts in different vm.Contexts,
aiding with the isolation that the vm module seeks to provide.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@addaleax addaleax added vm semver-minor worker labels Mar 7, 2019

@nodejs-github-bot

@nodejs-github-bot nodejs-github-bot added the lib / src label Mar 7, 2019

@addaleax addaleax force-pushed the addaleax:worker-messageport-move branch 4 times, most recently from b58679b to 1153455 Mar 7, 2019

@addaleax

This comment has been minimized.

Sign in to view
Copy link
Member Author

addaleax commented Mar 7, 2019

/cc @nodejs/workers

CI: https://ci.nodejs.org/job/node-test-pull-request/21303/
@jasnell

jasnell approved these changes Mar 7, 2019
View changes

@addaleax

This comment has been minimized.

Sign in to view
Copy link
Member Author

addaleax commented Mar 7, 2019

@chjj I’m not a 100 % sure but this might also make it easier to implement something closer to a “real” WebWorker-style Environment in Node.js, because you can control the global object and still have MessagePorts in it?

@addaleax addaleax requested a review from joyeecheung Mar 8, 2019

@BridgeAR

This comment has been minimized.

Sign in to view
Copy link
Member

BridgeAR commented Mar 8, 2019

CI https://ci.nodejs.org/job/node-test-pull-request/21329/

@BridgeAR BridgeAR added the author ready label Mar 8, 2019

@gengjiawen

gengjiawen reviewed Mar 8, 2019
View changes

Show resolved Hide resolved test/parallel/test-worker-message-port-move.js Outdated
@gengjiawen

gengjiawen approved these changes Mar 8, 2019
View changes

@addaleax addaleax force-pushed the addaleax:worker-messageport-move branch from e8b1b4a to bd0ef7a Mar 9, 2019

@addaleax

This comment has been minimized.

Sign in to view
Copy link
Member Author

addaleax commented Mar 9, 2019

Rebased, new CI: https://ci.nodejs.org/job/node-test-pull-request/21369/
@addaleax

This comment has been minimized.

Sign in to view
Copy link
Member Author

addaleax commented Mar 9, 2019

/cc @nodejs/workers
@joyeecheung

joyeecheung reviewed Mar 9, 2019
View changes

Copy link
Member

joyeecheung left a comment

The code generally LGTM but I have some questions
Show resolved Hide resolved src/api/environment.cc
Show resolved Hide resolved src/api/environment.cc
src/api/environment.cc
FIXED_ONE_BYTE_STRING(isolate, "node:per_context_binding_exports"));

Local<Value> existing_value;
if (!global->GetPrivate(context, key).ToLocal(&existing_value))

This comment has been minimized.

Sign in to view
Copy link
@joyeecheung

joyeecheung Mar 9, 2019 •
edited

Member

Can we use context->GetExtrasBindingObject() to store these somehow instead of using v8 privates (which are...an experimental feature. Use at your own risk.)? V8 already puts isTraceCategoryEnabled and trace in there.

This comment has been minimized.

Sign in to view
Copy link
@addaleax

addaleax Mar 10, 2019

Author Member

We’ve been using V8 privates extensively for a while, I don’t think they’re going away (and if they did we’d have bigger problems, e.g. N-API relies on them as well).

But yes, if you prefer, we could use the extras binding object for that … I feel like that’s mostly intended for actual v8-extras, but in theory nothing stops us, and I don’t mind making the switch if you prefer.

This comment has been minimized.

Sign in to view
Copy link
@joyeecheung

joyeecheung Mar 13, 2019

Member

@addaleax I don't have very strong opinions about the choices here...although I wonder what does this look like in the heap snapshot? Do they show up?

BTW, we probably need to do something similar for the Environment persistent handles (save them temporarily to the global proxy) when we implement snapshots...

@addaleax addaleax force-pushed the addaleax:worker-messageport-move branch 3 times, most recently from 76070c3 to 8937fd6 Mar 11, 2019

@addaleax

This comment has been minimized.

Sign in to view
Copy link
Member Author

addaleax commented Mar 12, 2019

Rebased again, CI: https://ci.nodejs.org/job/node-test-pull-request/21464/
@addaleax

This comment has been minimized.

Sign in to view
Copy link
Member Author

addaleax commented Mar 13, 2019

CI is green.

I’ll land this tomorrow if there are no objections, but I’d appreciate another review from @nodejs/workers.
@chjj

This comment has been minimized.

Sign in to view
Copy link
Contributor

chjj commented Mar 13, 2019

@addaleax, yeah, that's a possibility. Someone could replicate the web worker scope exactly with vm I suppose. It's not something I'm personally interested in though since I think I'm more focused on making the browser behave like node.js instead of the other way around (though, my approach seems to be less popular these days).

chjj added a commit to chjj/bthreads that referenced this pull request Mar 14, 2019

@chjj
bthreads: expose moveMessagePortToContext. 
- nodejs/node#26497
Verified
This commit was signed with a verified signature.
@chjj chjj Christopher Jeffrey (JJ)
GPG key ID: 8962AB9DE6666BBD Learn about signing commits
fb36b9d

addaleax added some commits Mar 7, 2019

@addaleax
src,lib: allow running multiple per-context files 
Create an `lib/internal/per_context/` directory that can
host multiple files which we execute for each context.

PR-URL: #26497
Reviewed-By: James M Snell <jasnell@gmail.com>
Verified
This commit was signed with a verified signature.
@addaleax addaleax Anna Henningsen
GPG key ID: 9C63F3A6CD2AD8F9 Learn about signing commits
92cddba
@addaleax
worker: implement worker.moveMessagePortToContext() 
This enables using `MessagePort`s in different `vm.Context`s,
aiding with the isolation that the `vm` module seeks to provide.

Refs: ayojs/ayo#111

PR-URL: #26497
Reviewed-By: James M Snell <jasnell@gmail.com>
Verified
This commit was signed with a verified signature.
@addaleax addaleax Anna Henningsen
GPG key ID: 9C63F3A6CD2AD8F9 Learn about signing commits
Loading status checks…
23bf4ce
@addaleax
src,lib: make DOMException available in all Contexts 
This allows using `DOMException` from Node.js code for any
`vm.Context`.

PR-URL: #26497
Reviewed-By: James M Snell <jasnell@gmail.com>
Verified
This commit was signed with a verified signature.
@addaleax addaleax Anna Henningsen
GPG key ID: 9C63F3A6CD2AD8F9 Learn about signing commits
Loading status checks…
a15fd18

@addaleax addaleax force-pushed the addaleax:worker-messageport-move branch from 8937fd6 to 23bf4ce Mar 15, 2019

@addaleax

This comment has been minimized.

Sign in to view
Copy link
Member Author

addaleax commented Mar 15, 2019

Sigh… Needed to update this because of test/parallel/test-bootstrap-modules.js.

CI: https://ci.nodejs.org/job/node-test-pull-request/21564/
@addaleax

This comment has been minimized.

Sign in to view
Copy link
Member Author

addaleax commented Mar 15, 2019

Landed in 0752a18...fe8972a

@addaleax addaleax closed this Mar 15, 2019

@addaleax addaleax deleted the addaleax:worker-messageport-move branch Mar 15, 2019

addaleax added a commit that referenced this pull request Mar 15, 2019

@addaleax
src,lib: allow running multiple per-context files 
Create an `lib/internal/per_context/` directory that can
host multiple files which we execute for each context.

PR-URL: #26497
Reviewed-By: James M Snell <jasnell@gmail.com>
Verified
This commit was signed with a verified signature.
@addaleax addaleax Anna Henningsen
GPG key ID: 9C63F3A6CD2AD8F9 Learn about signing commits
7e2088f

addaleax added a commit that referenced this pull request Mar 15, 2019

@addaleax
src,lib: make DOMException available in all Contexts 
This allows using `DOMException` from Node.js code for any
`vm.Context`.

PR-URL: #26497
Reviewed-By: James M Snell <jasnell@gmail.com>
Verified
This commit was signed with a verified signature.
@addaleax addaleax Anna Henningsen
GPG key ID: 9C63F3A6CD2AD8F9 Learn about signing commits
b0de48e

addaleax added a commit that referenced this pull request Mar 15, 2019

@addaleax
worker: implement worker.moveMessagePortToContext() 
This enables using `MessagePort`s in different `vm.Context`s,
aiding with the isolation that the `vm` module seeks to provide.

Refs: ayojs/ayo#111

PR-URL: #26497
Reviewed-By: James M Snell <jasnell@gmail.com>
Verified
This commit was signed with a verified signature.
@addaleax addaleax Anna Henningsen
GPG key ID: 9C63F3A6CD2AD8F9 Learn about signing commits
Loading status checks…
fe8972a

Drieger added a commit to Drieger/node that referenced this pull request Mar 22, 2019

@addaleax @Drieger
src,lib: allow running multiple per-context files 
Create an `lib/internal/per_context/` directory that can
host multiple files which we execute for each context.

PR-URL: nodejs#26497
Reviewed-By: James M Snell <jasnell@gmail.com>
75c2399

Drieger added a commit to Drieger/node that referenced this pull request Mar 22, 2019

@addaleax @Drieger
src,lib: make DOMException available in all Contexts 
This allows using `DOMException` from Node.js code for any
`vm.Context`.

PR-URL: nodejs#26497
Reviewed-By: James M Snell <jasnell@gmail.com>
d26b2ea

Drieger added a commit to Drieger/node that referenced this pull request Mar 22, 2019

@addaleax @Drieger
worker: implement worker.moveMessagePortToContext() 
This enables using `MessagePort`s in different `vm.Context`s,
aiding with the isolation that the `vm` module seeks to provide.

Refs: ayojs/ayo#111

PR-URL: nodejs#26497
Reviewed-By: James M Snell <jasnell@gmail.com>
6878085

targos added a commit that referenced this pull request Mar 27, 2019

@addaleax @targos
src,lib: allow running multiple per-context files 
Create an `lib/internal/per_context/` directory that can
host multiple files which we execute for each context.

PR-URL: #26497
Reviewed-By: James M Snell <jasnell@gmail.com>
Verified
This commit was signed with a verified signature.
@targos targos Michaël Zasso
GPG key ID: 770F7A9A5AE15600 Learn about signing commits
e044563

targos added a commit that referenced this pull request Mar 27, 2019

@addaleax @targos
src,lib: make DOMException available in all Contexts 
This allows using `DOMException` from Node.js code for any
`vm.Context`.

PR-URL: #26497
Reviewed-By: James M Snell <jasnell@gmail.com>
Verified
This commit was signed with a verified signature.
@targos targos Michaël Zasso
GPG key ID: 770F7A9A5AE15600 Learn about signing commits
1e669b2

targos added a commit that referenced this pull request Mar 27, 2019

@addaleax @targos
worker: implement worker.moveMessagePortToContext() 
This enables using `MessagePort`s in different `vm.Context`s,
aiding with the isolation that the `vm` module seeks to provide.

Refs: ayojs/ayo#111

PR-URL: #26497
Reviewed-By: James M Snell <jasnell@gmail.com>
Verified
This commit was signed with a verified signature.
@targos targos Michaël Zasso
GPG key ID: 770F7A9A5AE15600 Learn about signing commits
3c6f12c

@targos targos referenced this pull request Mar 27, 2019

Merged

v11.13.0 release proposal #26949

targos added a commit that referenced this pull request Mar 28, 2019

@targos
2019-03-28, Version 11.13.0 (Current) 
Notable changes:

* crypto
  * Allow deriving public from private keys (Tobias Nießen)
    [#26278](#26278).
* events
  * Added a `once` function to use `EventEmitter` with promises
    (Matteo Collina) [#26078](#26078).
* tty
  * Added a `hasColors` method to `WriteStream` (Ruben Bridgewater)
    [#26247](#26247).
  * Added NO_COLOR and FORCE_COLOR support (Ruben Bridgewater)
    [#26485](#26485).
* v8
  * Added `v8.getHeapSnapshot` and `v8.writeHeapSnapshot` to generate snapshots
    in the format used by tools such as Chrome DevTools (James M Snell)
    [#26501](#26501).
* worker
  * Added `worker.moveMessagePortToContext`. This enables using MessagePorts in
    different vm.Contexts, aiding with the isolation that the vm module seeks to
    provide (Anna Henningsen)
    [#26497](#26497).
* C++ API
  * `AddPromiseHook` is now deprecated. This API was added to fill an use case
    that is served by `async_hooks`, since that has `Promise` support
    (Anna Henningsen) [#26529](#26529).
  * Added a `Stop` API to shut down Node.js while it is running
    (Gireesh Punathil) [#21283](#21283).
* meta
  * [Gireesh Punathil](https://github.com/gireeshpunathil) is now a member of
    the Technical Steering Committee
    [#26657](#26657).
  * Added [Yongsheng Zhang](https://github.com/ZYSzys) to collaborators
    [#26730](#26730).

PR-URL: #26949
Verified
This commit was signed with a verified signature.
@targos targos Michaël Zasso
GPG key ID: 770F7A9A5AE15600 Learn about signing commits
Loading status checks…
29867f3

targos added a commit that referenced this pull request Mar 28, 2019

@targos
2019-03-28, Version 11.13.0 (Current) 
Notable changes:

* crypto
  * Allow deriving public from private keys (Tobias Nießen)
    [#26278](#26278).
* events
  * Added a `once` function to use `EventEmitter` with promises
    (Matteo Collina) [#26078](#26078).
* tty
  * Added a `hasColors` method to `WriteStream` (Ruben Bridgewater)
    [#26247](#26247).
  * Added NO_COLOR and FORCE_COLOR support (Ruben Bridgewater)
    [#26485](#26485).
* v8
  * Added `v8.getHeapSnapshot` and `v8.writeHeapSnapshot` to generate snapshots
    in the format used by tools such as Chrome DevTools (James M Snell)
    [#26501](#26501).
* worker
  * Added `worker.moveMessagePortToContext`. This enables using MessagePorts in
    different vm.Contexts, aiding with the isolation that the vm module seeks to
    provide (Anna Henningsen)
    [#26497](#26497).
* C++ API
  * `AddPromiseHook` is now deprecated. This API was added to fill an use case
    that is served by `async_hooks`, since that has `Promise` support
    (Anna Henningsen) [#26529](#26529).
  * Added a `Stop` API to shut down Node.js while it is running
    (Gireesh Punathil) [#21283](#21283).
* meta
  * [Gireesh Punathil](https://github.com/gireeshpunathil) is now a member of
    the Technical Steering Committee
    [#26657](#26657).
  * Added [Yongsheng Zhang](https://github.com/ZYSzys) to collaborators
    [#26730](#26730).

PR-URL: #26949
Verified
This commit was signed with a verified signature.
@targos targos Michaël Zasso
GPG key ID: 770F7A9A5AE15600 Learn about signing commits
Loading status checks…
2c73868

BethGriggs added a commit that referenced this pull request Apr 5, 2019

@targos @BethGriggs
2019-03-28, Version 11.13.0 (Current) 
Notable changes:

* crypto
  * Allow deriving public from private keys (Tobias Nießen)
    [#26278](#26278).
* events
  * Added a `once` function to use `EventEmitter` with promises
    (Matteo Collina) [#26078](#26078).
* tty
  * Added a `hasColors` method to `WriteStream` (Ruben Bridgewater)
    [#26247](#26247).
  * Added NO_COLOR and FORCE_COLOR support (Ruben Bridgewater)
    [#26485](#26485).
* v8
  * Added `v8.getHeapSnapshot` and `v8.writeHeapSnapshot` to generate snapshots
    in the format used by tools such as Chrome DevTools (James M Snell)
    [#26501](#26501).
* worker
  * Added `worker.moveMessagePortToContext`. This enables using MessagePorts in
    different vm.Contexts, aiding with the isolation that the vm module seeks to
    provide (Anna Henningsen)
    [#26497](#26497).
* C++ API
  * `AddPromiseHook` is now deprecated. This API was added to fill an use case
    that is served by `async_hooks`, since that has `Promise` support
    (Anna Henningsen) [#26529](#26529).
  * Added a `Stop` API to shut down Node.js while it is running
    (Gireesh Punathil) [#21283](#21283).
* meta
  * [Gireesh Punathil](https://github.com/gireeshpunathil) is now a member of
    the Technical Steering Committee
    [#26657](#26657).
  * Added [Yongsheng Zhang](https://github.com/ZYSzys) to collaborators
    [#26730](#26730).

PR-URL: #26949
Verified
This commit was signed with a verified signature.
@BethGriggs BethGriggs Bethany Nicolle Griggs
GPG key ID: D7062848A1AB005C Learn about signing commits
473b162
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.