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: allow synchronously draining message ports #26686

Closed
wants to merge 1 commit into from

Conversation

addaleax
Copy link
Member

In combination with Atomics, this makes it possible to implement
generic synchronous functionality, e.g. importScript(), in Workers
purely by communicating with other threads.

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

In combination with Atomics, this makes it possible to implement
generic synchronous functionality, e.g. `importScript()`, in Workers
purely by communicating with other threads.
@addaleax addaleax added semver-minor PRs that contain new features and should be released in the next minor version. worker Issues and PRs related to Worker support. labels Mar 15, 2019
@devsnek
Copy link
Member

devsnek commented Mar 15, 2019

i wonder if this could be made more granular, like getPendingMessage(port) or something.

@addaleax
Copy link
Member Author

@devsnek Sure. Would you prefer that?

@devsnek
Copy link
Member

devsnek commented Mar 15, 2019

@addaleax personally i've had a use case for a while where grabbing individual messages would have been useful. while drain uses the existing handlers, i'm imaging that getPendingMessage would take from the queue and not emit an event for it.

at a certain point you could argue for both to exist so i'm not really sure.

@addaleax
Copy link
Member Author

@devsnek It would add some additional complexity, but it seems pretty doable and also fits my personal use case, so … ¯\_(ツ)_/¯

@BridgeAR
Copy link
Member

Code LGTM. To me Gus's suggestion sounds best though. I am curious about use cases which could not be solved with the granular implementation. I am +1 to landing this as is if there are any other use cases and otherwise +0.

@BridgeAR
Copy link
Member

BridgeAR commented Apr 3, 2019

Ping @addaleax

@@ -1,6 +1,7 @@
'use strict';

const {
drainMessagePort,
Copy link
Member

Choose a reason for hiding this comment

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

Nit: FWIW, would it be better if we require drainMessagePort from internal/worker/io directly instead of internal/worker (then doesn't need to export in internal/worker), since it was exported from internal/worker/io originally.

module.exports = {
drainMessagePort,

@addaleax
Copy link
Member Author

I’ll follow the suggestions above and am working on implementing the single-message approach. That’s going to be a very different PR in terms of content, though, so I’ll close this one – thanks for the feedback!

@addaleax addaleax closed this Apr 15, 2019
@addaleax addaleax deleted the message-port-sync-drain branch April 17, 2019 23:01
addaleax added a commit to addaleax/node that referenced this pull request May 19, 2019
In combination with Atomics, this makes it possible to implement
generic synchronous functionality, e.g. `importScript()`, in Workers
purely by communicating with other threads.

This is a continuation of nodejs#26686,
where a preference for a solution was voiced that allowed reading
individual messages, rather than emitting all messages through events.
addaleax added a commit that referenced this pull request May 19, 2019
In combination with Atomics, this makes it possible to implement
generic synchronous functionality, e.g. `importScript()`, in Workers
purely by communicating with other threads.

This is a continuation of #26686,
where a preference for a solution was voiced that allowed reading
individual messages, rather than emitting all messages through events.

PR-URL: #27294
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
targos pushed a commit that referenced this pull request May 20, 2019
In combination with Atomics, this makes it possible to implement
generic synchronous functionality, e.g. `importScript()`, in Workers
purely by communicating with other threads.

This is a continuation of #26686,
where a preference for a solution was voiced that allowed reading
individual messages, rather than emitting all messages through events.

PR-URL: #27294
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-minor PRs that contain new features and should be released in the next minor version. worker Issues and PRs related to Worker support.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants