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

doc: clarify synchronous blocking of worker stdio #38658

Closed
wants to merge 2 commits into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented May 12, 2021

Fixes: #25630
Signed-off-by: James M Snell jasnell@gmail.com

@github-actions github-actions bot added doc Issues and PRs related to the documentations. worker Issues and PRs related to Worker support. labels May 12, 2021
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

I mean, this is true for literally all Node.js events (or any other runtime with an event loop). I don’t see why we would want to document this for MessagePorts specifically, unless we plan to add a section for this to literally every other event as well.

The issue this references seems to be specifically about the fact that stdio from Workers uses message passing under the hood, and can get blocked by synchronous code on the receiving end, not the sending one. It’s true that that’s not obvious, so if we do document anything, that is what we should document.

doc/api/worker_threads.md Outdated Show resolved Hide resolved
@jasnell
Copy link
Member Author

jasnell commented May 13, 2021

The issue this references seems to be specifically about the fact that stdio from Workers uses message passing under the hood, and can get blocked by synchronous code on the receiving end, not the sending one. It’s true that that’s not obvious, so if we do document anything, that is what we should document.

Yeah, I was struggling with that a bit also but I think you definitely capture it here... will update :-)

Fixes: nodejs#25630
Signed-off-by: James M Snell <jasnell@gmail.com>
@jasnell jasnell force-pushed the doc-messageport-blocking branch from a2e26c4 to 01396c4 Compare May 13, 2021 04:02
@jasnell jasnell requested a review from addaleax May 13, 2021 04:02
@jasnell jasnell changed the title doc: clarify synchronous blocking of MessagePort doc: clarify synchronous blocking of worker stdio May 13, 2021
@jasnell jasnell added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 13, 2021
doc/api/worker_threads.md Outdated Show resolved Hide resolved
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
jasnell added a commit that referenced this pull request May 17, 2021
Fixes: #25630
Signed-off-by: James M Snell <jasnell@gmail.com>

PR-URL: #38658
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@jasnell
Copy link
Member Author

jasnell commented May 17, 2021

Landed in 61a67c1

@jasnell jasnell closed this May 17, 2021
targos pushed a commit that referenced this pull request May 18, 2021
Fixes: #25630
Signed-off-by: James M Snell <jasnell@gmail.com>

PR-URL: #38658
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
targos pushed a commit that referenced this pull request May 30, 2021
Fixes: #25630
Signed-off-by: James M Snell <jasnell@gmail.com>

PR-URL: #38658
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
targos pushed a commit that referenced this pull request Jun 5, 2021
Fixes: #25630
Signed-off-by: James M Snell <jasnell@gmail.com>

PR-URL: #38658
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
targos pushed a commit that referenced this pull request Jun 5, 2021
Fixes: #25630
Signed-off-by: James M Snell <jasnell@gmail.com>

PR-URL: #38658
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
targos pushed a commit that referenced this pull request Jun 11, 2021
Fixes: #25630
Signed-off-by: James M Snell <jasnell@gmail.com>

PR-URL: #38658
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. doc Issues and PRs related to the documentations. worker Issues and PRs related to Worker support.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

worker_threads: worker.postMessage does not get executed without exiting synchronous call stack
3 participants