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: do not throw on property access or postMessage after termination #25871

Closed
wants to merge 2 commits into from

Conversation

@chjj
Copy link
Contributor

commented Feb 1, 2019

Worker#postMessage, Worker#stdin, Worker#stdout, and Worker#stderr currently throw if called/accessed after a worker is terminated.

postMessage silently fails in the browser after termination. It also seems cleaner if the stdio streams are set to null after termination. This PR implements that behavior.

@addaleax

This comment has been minimized.

@devsnek
devsnek approved these changes Feb 1, 2019
@sam-github

This comment has been minimized.

Copy link
Member

commented Feb 1, 2019

It also seems cleaner if the stdio streams are set to null after termination.

I don't know, it seems better to me if they continue to exist, but are closed. null will make property access start to throw, which is what is happening now, except the error will be more mysterious.

EDIT: sorry, ignore this. I thought this was the cluster Worker, but its not. I have no opinion on WebWorkers. The API is supposed to be standardized, does the standard not address this?

@addaleax addaleax removed the author ready label Feb 1, 2019

@sam-github

This comment has been minimized.

Copy link
Member

commented Feb 1, 2019

I edited my comment above, but then remembered edits don't cause notifications. See ----^

@chjj

This comment has been minimized.

Copy link
Contributor Author

commented Feb 1, 2019

@sam-github, I'm not sure what the standard says, but chromium 71 fails silently when calling Worker#postMessage after termination (the same is true of MessagePort instances).

stdin, stdout, and stderr are specific to node so I guess it's up to us to do what we want with them. I feel like it's bad practice to have getters which throw.

@chjj

This comment has been minimized.

Copy link
Contributor Author

commented Feb 1, 2019

Mmm, will modify to make the linter happy.

@chjj

This comment has been minimized.

Copy link
Contributor Author

commented Feb 1, 2019

Should be fixed. I can squash and push again if that's preferred.

@lpinca
lpinca approved these changes Feb 1, 2019

// Sanity check.
assert.strictEqual(w.threadId, -1);
assert.strictEqual(w.stdin, null);

This comment has been minimized.

Copy link
@lpinca

lpinca Feb 1, 2019

Member

Nit: this also checks that w.stdin does not throw. Can the one above be removed?

@cjihrig
cjihrig approved these changes Feb 3, 2019
@hiroppy
hiroppy approved these changes Feb 4, 2019
@addaleax

This comment has been minimized.

Copy link
Member

commented Feb 4, 2019

@addaleax

This comment has been minimized.

Copy link
Member

commented Feb 4, 2019

@chjj I think @sam-github’s suggestion for stdio was to make the getters not throw by removing the this[kParentSideStdio] = null line, rather than returning null from the getters, so that they always return the same values as before the worker exited? I like that idea.

@jasnell
jasnell approved these changes Feb 5, 2019
@addaleax

This comment has been minimized.

Copy link
Member

commented Feb 6, 2019

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

If this isn’t addressed in this PR, I think I’d like to land this and then apply @sam-github’s suggestion and make sure one PR doesn’t get released without the other.

addaleax added a commit to addaleax/node that referenced this pull request Feb 9, 2019
worker: no throw on property access/postMessage after termination
PR-URL: nodejs#25871
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Yuta Hiroto <hello@hiroppy.me>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax added a commit to addaleax/node that referenced this pull request Feb 9, 2019
worker: keep stdio after exit
This addresses review comments from
nodejs#25871.

Refs: nodejs#25871
@addaleax addaleax referenced this pull request Feb 9, 2019
3 of 3 tasks complete
@addaleax

This comment has been minimized.

Copy link
Member

commented Feb 9, 2019

Landed in 2fc759b, see #26017 for the follow-up PR

@addaleax addaleax closed this Feb 9, 2019

targos added a commit that referenced this pull request Feb 10, 2019
worker: no throw on property access/postMessage after termination
PR-URL: #25871
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Yuta Hiroto <hello@hiroppy.me>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax added a commit that referenced this pull request Feb 12, 2019
worker: keep stdio after exit
This addresses review comments from
#25871.

Refs: #25871

PR-URL: #26017
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
addaleax added a commit that referenced this pull request Feb 13, 2019
worker: keep stdio after exit
This addresses review comments from
#25871.

Refs: #25871

PR-URL: #26017
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@targos targos referenced this pull request Feb 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
10 participants
You can’t perform that action at this time.