Skip to content
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

Projects
None yet
7 participants
@addaleax
Copy link
Member

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 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.

@jasnell

jasnell approved these changes Mar 7, 2019

@addaleax

This comment has been minimized.

Copy link
Member Author

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.

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

@addaleax

This comment has been minimized.

Copy link
Member Author

commented Mar 9, 2019

@addaleax

This comment has been minimized.

Copy link
Member Author

commented Mar 9, 2019

@joyeecheung
Copy link
Member

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
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.

Copy link
@joyeecheung

joyeecheung Mar 9, 2019

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.

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.

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.

Copy link
Member Author

commented Mar 12, 2019

@addaleax

This comment has been minimized.

Copy link
Member Author

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.

Copy link
Contributor

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

addaleax added some commits 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.

PR-URL: #26497
Reviewed-By: James M Snell <jasnell@gmail.com>
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>
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>

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

@addaleax

This comment has been minimized.

Copy link
Member Author

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.

Copy link
Member Author

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

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>

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

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>

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

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>

Drieger added a commit to Drieger/node that referenced this pull request Mar 22, 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.

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

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

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>

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

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>

targos added a commit that referenced this pull request Mar 27, 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.

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

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

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>

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

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>

@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

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

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

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

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

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
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.