-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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
Conversation
In combination with Atomics, this makes it possible to implement generic synchronous functionality, e.g. `importScript()`, in Workers purely by communicating with other threads.
i wonder if this could be made more granular, like |
@devsnek Sure. Would you prefer that? |
@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 at a certain point you could argue for both to exist so i'm not really sure. |
@devsnek It would add some additional complexity, but it seems pretty doable and also fits my personal use case, so … ¯\_(ツ)_/¯ |
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. |
Ping @addaleax |
@@ -1,6 +1,7 @@ | |||
'use strict'; | |||
|
|||
const { | |||
drainMessagePort, |
There was a problem hiding this comment.
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.
node/lib/internal/worker/io.js
Lines 243 to 244 in d5a5b99
module.exports = { | |
drainMessagePort, |
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! |
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.
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>
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>
In combination with Atomics, this makes it possible to implement
generic synchronous functionality, e.g.
importScript()
, in Workerspurely by communicating with other threads.
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes