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

lib: worker fix workers isolation #25202

Closed
wants to merge 1 commit into from

Conversation

sagitsofan
Copy link
Contributor

@sagitsofan sagitsofan commented Dec 24, 2018

lib: fix workers isolation

Added strict mode inside the worker script.

Fixes: #24947

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the worker Issues and PRs related to Worker support. label Dec 24, 2018
Copy link
Contributor

@tshemsedinov tshemsedinov left a comment

Choose a reason for hiding this comment

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

That's not good practice to duplicate PRs. Instead of closing #25196 it would be better to rebase it. New PR contains 3 commits including merge commit and you need to delete merge commit and squash everything to a single commit. If you have problems with rebase, other contributors can help you but don't open new PR. I am not sure you need to reopen and rebase previous PR or rebase this one. @lundibundi can you please help us?

@Trott
Copy link
Member

Trott commented Dec 24, 2018

To keep things moving, I rebased and force pushed to the branch.

(If it helps anywone for the future, I used git rebase -i upstream/master and used fixup on all commits but the first one to squash everything into the fist commit. Then I use git push --force-with-lease to push the branch back up to @sagitsofan's remote/branch, updating the PR.)

Before the force push, I ran git commit --amend to edit the commit message slightly to more strictly comply with our commit message requirements. (It was OK the way it was, but the subsystem can be narrower and the first word after the subsystem is preferred to be an imperative verb. These are no big deal, but I figured while I was there, I might as well tidy that up too.)

@sagitsofan
Copy link
Contributor Author

sagitsofan commented Dec 24, 2018

@Trott @tshemsedinov Thanks, i will pay attention for next time.

Added strict mode inside the worker script.

Fixes: nodejs#24947
@Trott
Copy link
Member

Trott commented Dec 24, 2018

Copy link
Member

@devsnek devsnek left a comment

Choose a reason for hiding this comment

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

This doesn't actually fix the issue, and will probably break people who are expecting to use sloppy mode.

Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

Needs a test.

@sagitsofan
Copy link
Contributor Author

sagitsofan commented Dec 25, 2018

So i made some more tests and it seems that the strict mode is really not fixing the issue.
Should the appropriate solution needs to be a regex search on the input script or some other solution?

@devsnek
Copy link
Member

devsnek commented Dec 25, 2018

@sagitsofan the setter and deleter need to be conditionally installed depending on if its being run in the main thread or worker thread.

node/src/node_env_var.cc

Lines 208 to 209 in 77db1e7

env_proxy_template->SetHandler(NamedPropertyHandlerConfiguration(
EnvGetter, EnvSetter, EnvQuery, EnvDeleter, EnvEnumerator, data));

@sagitsofan
Copy link
Contributor Author

sagitsofan commented Dec 28, 2018

the setter and deleter need to be conditionally installed depending on if its being run in the main thread or worker thread.
@devsnek
Should the check of the main thread needs to be under worker.js or node_env_var.cc?

@addaleax
Copy link
Member

I don’t think this PR will end up being merged, so I’ll close this out. Sorry, and thanks for the PR!

@addaleax addaleax closed this Aug 25, 2019
@addaleax addaleax added the stalled Issues and PRs that are stalled. label Aug 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stalled Issues and PRs that are stalled. worker Issues and PRs related to Worker support.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(experimental) worker isolation
6 participants