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

vm: refactor to use more primordials #36023

Closed
wants to merge 1 commit into from

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Nov 7, 2020

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

@nodejs-github-bot nodejs-github-bot added the vm Issues and PRs related to the vm subsystem. label Nov 7, 2020
@aduh95 aduh95 added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 7, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 7, 2020
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot
Copy link
Collaborator

@aduh95 aduh95 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 7, 2020
@aduh95 aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 14, 2020
@github-actions github-actions bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 14, 2020
@github-actions
Copy link
Contributor

Landed in 8297860...1d02a35

nodejs-github-bot pushed a commit that referenced this pull request Nov 14, 2020
PR-URL: #36023
Reviewed-By: Rich Trott <rtrott@gmail.com>
@github-actions github-actions bot closed this Nov 14, 2020
@aduh95 aduh95 deleted the vm-primordials branch November 14, 2020 14:49
codebytere pushed a commit that referenced this pull request Nov 22, 2020
PR-URL: #36023
Reviewed-By: Rich Trott <rtrott@gmail.com>
@codebytere codebytere mentioned this pull request Nov 22, 2020
BethGriggs pushed a commit that referenced this pull request Dec 9, 2020
PR-URL: #36023
Reviewed-By: Rich Trott <rtrott@gmail.com>
BethGriggs pushed a commit that referenced this pull request Dec 10, 2020
PR-URL: #36023
Reviewed-By: Rich Trott <rtrott@gmail.com>
@BethGriggs BethGriggs mentioned this pull request Dec 10, 2020
BethGriggs pushed a commit that referenced this pull request Dec 15, 2020
PR-URL: #36023
Reviewed-By: Rich Trott <rtrott@gmail.com>
@rubenstolk
Copy link

Great - an automatic upgrade from 14.15.1 to 14.15.2 started crashing our production environment. Lesson learnt, don't accept minor updates from Node just like that...

@aduh95
Copy link
Contributor Author

aduh95 commented Dec 17, 2020

@rubenstolk Do you want to open an issue describing the bug?

@rubenstolk
Copy link

rubenstolk commented Dec 17, 2020

@aduh95 I don't know if it's a bug. While running this inside a vm script:

(req) { // an express request object
  const dummyReq = { ...req, model: undefined }; // create a copy of `req` with some modifications
  // Here dummyReq.headers has become undefined
}

This piece of code has been working for years, but since 14.15.2 the dummyReq doesn't have headers anymore...

@rubenstolk
Copy link

Hmm looking deeper into this, I don't think it has to do with the vm, I made an assumption here. The behavior is also there when running outside of vm.

@aduh95
Copy link
Contributor Author

aduh95 commented Dec 17, 2020

@rubenstolk I'm not able to reproduce the bug:

$ node --version
v14.15.2
$ node -p 'new vm.Script("\
const req = { headers: { headerKey: `value` }, method : `value` };\
const dummy = { ...req, method: undefined };\
dummy.headers\
").runInNewContext()'
{ headerKey: 'value' }

I'm seeing the expected { headerKey: 'value' } on stdout…

Opening a new issue would make your problem likely to be fixed quicker as more people would notice the issue and help fix it – only people already subscribed to this thread are seeing our messages here.

@rubenstolk
Copy link

@aduh95 this is what fails, so it's NOT because of vm:

require('http').createServer((req, res) => {
  const dummyReq = { ...req };
  console.log(!!req.headers, !!dummyReq.headers);
  res.writeHead(200, { 'Content-Type': 'text/plain' });
  res.end('ok');
}).listen(8000);

@ExE-Boss
Copy link
Contributor

ExE-Boss commented Dec 17, 2020

@rubenstolk
Do note that object spread only copies own enumerable properties, e.g.:

const result = { 
	...(Object.create(
		{
			prototypeProperty: "foo",
		},
		{
			nonEnumerableProperty: {
				configurable: true,
				enumerable: false,
				writable: true,
				value: 123,
			}
		}
	)),
};

Results in an empty object with a prototype of Object.prototype.

@rubenstolk
Copy link

Let's continue the conversation here: #36550, would be interested to understand why the behavior has changed @ExE-Boss !

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. vm Issues and PRs related to the vm subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants