vm: update source, related basic & cached data tests for ArrayBuffer #22921

BeniCheni · 2018-09-18T05:40:31Z

Referring to the comment in "mentor-available" #1826, this PR tries out the vm item from the checklist of the comment:

accept ArrayBufferView type(s) in place of Uint8Array in the source
related test code in test-vm-basic.js & test-vm-cached-data.js.

(P.S.: if any potential test case(s) or vm doc update is missed, please advise. Thanks!)

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

targos

The native code must be changed to handle the new allowed types.

targos · 2018-09-18T11:01:40Z

Example showing that the C++ part is not ready for it:

'use strict';

const vm = require('vm');

let script = new vm.Script('var x = 0;', { produceCachedData: true });
const cachedData = script.cachedData;

new vm.Script('var x = 0;', { cachedData: new Uint16Array(cachedData.buffer) } );

$ ./node test.js
./node[10954]: ../src/node_contextify.cc:685:static void node::contextify::ContextifyScript::New(const v8::FunctionCallbackInfo<v8::Value>&): Assertion `args[4]->IsUint8Array()' failed.
 1: 0x8dbd90 node::Abort() [./node]
 2: 0x8dbe65  [./node]
 3: 0x910fea node::contextify::ContextifyScript::New(v8::FunctionCallbackInfo<v8::Value> const&) [./node]
 4: 0xb6e1f1  [./node]
 5: 0xb6e7bf  [./node]
 6: 0x1c3b281cfc3d 
[1]    10954 abort (core dumped)  ./node test.js

BeniCheni · 2018-09-26T04:27:17Z

Thank you, @ targos / ping reviewer(s).

Learned about the “vm” module & dug deeper in the node_contextify.cc code, based on the stack-trace error from your helpful test case, and able to use ArrayBufferView/IsArrayBufferView in places of Uint8Array/IsUint8Array. Please review at your convenience,

Tested w. the following test snippet:

‘use strict';

const vm = require('vm');

let script = new vm.Script('var x = 0;', { produceCachedData: true });
const cachedData = script.cachedData;

new vm.Script('var x = 0;', { cachedData: new Uint16Array(cachedData.buffer) } )

BeniCheni · 2018-10-01T04:02:44Z

Ping/Dear reviewers,

Friendly reminder that - Made updates according per previous review comments, & was able to passed the suggested “vm” test w. cached data. Please review again at convenience.

lundibundi

I think change to the doc is missing here https://github.com/nodejs/node/blob/master/doc/api/vm.md#new-vmscriptcode-options and here https://github.com/nodejs/node/blob/master/doc/api/vm.md#vmcompilefunctioncode-params-options.

Also, I don't see any tests for TypedArray or DataView as cachedData, is there any?

BeniCheni · 2018-10-03T16:33:46Z

@lundibundi,

I think change to the doc is missing here https://github.com/nodejs/node/blob/master/doc/api/vm.md#new-vmscriptcode-options and here https://github.com/nodejs/node/blob/master/doc/api/vm.md#vmcompilefunctioncode-params-options.

Updated the "cachedData" description for new-vmscriptcode-options & vmcompilefunctioncode-params-options docs. Thank you!

Also, I don't see any tests for TypedArray or DataView as cachedData, is there any?

The updated test in test/parallel/test-vm-cached-data.js for ERR_INVALID_ARG_TYPE should cover cachedData with the validation of TypedArray & DataView type error.

Please guide me other test(s) that'd add test for TypedArray or DataView as cachedData?

{
  code: 'ERR_INVALID_ARG_TYPE',
  type: TypeError,
-message: /must be one of type Buffer or Uint8Array/
+message: /must be one of type Buffer, TypedArray, or DataView/
}

lundibundi · 2018-10-03T17:51:56Z

Thanks for the swift changes =). It seems there is no reasonable way to test for cachedData besides simple arg checks, so it should be fine.

Though, I cannot understand how can we actually get the cachedData in anything besides Buffer. IIRC https://nodejs.org/api/vm.html#vm_script_createcacheddata is the only way to get cachedData therefore is there any sense of having cachedData available as TypedArray or DataView?

Also, this line https://github.com/nodejs/node/pull/22921/files#diff-0cf206672499c2f86db4ffb0cc5b668bR683 and this https://github.com/nodejs/node/pull/22921/files#diff-0cf206672499c2f86db4ffb0cc5b668bR1019 extracts data as uint8_t will this still be correct if we pass i.e. Int32Array (it should work but would that be correct)?

BeniCheni · 2018-10-04T01:46:06Z

@lundibundi and/or other dear reviewers,

Per these two topics, I'd need some guideline to know how to update the PR to move forward. Would you mind advising?

...sense of having cachedData available as TypedArray or DataView"

...extracts data as uint8_t will this still be correct if we pass i.e. Int32Array (it should work but would that be correct)?

BeniCheni · 2018-10-06T14:41:24Z

Ping / dear reviewers,

Would you mind guiding this PR for next step based on the questions in last comment? (Hoping the PR would not become stale; thank you!)

lundibundi · 2018-10-06T20:33:28Z

ping @addaleax @jasnell @targos as the ones who reviewed this could you provide some input on #22921 (comment)?

Just to be clear: I'm not blocking this, my concern is - maybe it's worth changing something here for it to become a bit more relevant.

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

TimothyGu · 2018-10-06T21:11:37Z

I don’t quite see any tests for the newly supported types. You could use the common.getArrayBufferViews() function to make sure all TypedArray and DataView types are actually supported. Check out https://github.com/nodejs/node/blob/master/test/parallel/test-fs-write-file-typedarrays.js for an example

BeniCheni · 2018-10-21T15:58:51Z

Ping / dear reviewers,

Per the suggestion in last comment, I've rebased & updated the tests in "test-vm-cached-data.js" to use common.getArrayBufferViews() function, to ensure all TypedArray and DataView types are actually supported. Please review again at convenience.

(P.S.: sorry about the delayed update. Was in Node+JS Interactive (glad to meet some of you in person), & a busy week at work after the conf. Looking forward to your review again to pick back up this PR, to work towards landing/closure. 🙏 )

lundibundi · 2018-10-23T12:55:52Z

test/parallel/test-vm-cached-data.js

+  for (const parsedData of common.getArrayBufferViews(data)) {
+    // It should consume code cache 
+    const script = new vm.Script(source, {
+      cachedData: parsedData


Nit: I'd prefer to just rename parsedData -> cachedData and the this line will become just

Suggested change

cachedData: parsedData

cachedData

lundibundi · 2018-10-23T12:56:39Z

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

Also, ping @targos, I believe your concerns were addressed.

native layer was updated and tests added

BeniCheni · 2018-10-23T14:45:47Z

@lundibundi, updated to use cachedData, since the term is indeed more contextually aligned than ~~parsedData~~. Please review again at convenience. Thank you!

(P.S: also thank you @targos for reviewing the "native layer" part!)

BeniCheni · 2018-10-30T12:40:36Z

@lundibundi & reviewers,

Friendly reminder that Updated to use cachedData variable name per nit in last comment.

Would you mind reviewing to see if the PR is okay for “author ready” label, since all previous feedback & advice have been addressed? Thank you!

refack · 2018-10-30T15:14:52Z

https://ci.nodejs.org/job/node-test-pull-request/18232/

In vm module, accept ArrayBuffer in place of Uint8Array in the source code & related test code in "test-vm-basic.js" & "test-vm-cached-data.js", per the "vm" item in the checklist of the comment in #1826. Refs: #1826 (comment)

Use ArrayBufferView/IsArrayBufferView in places of Uint8Array/IsUint8Array, in the node_contextify.cc source code. Refs: #22921 (comment)

Update the "cachedData" description with support of TypedArray or DataView support in “new vm.Script” & “vm.compileFunction”.

refack · 2018-11-01T21:11:39Z

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

Trott · 2018-11-02T02:32:37Z

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

refack · 2018-11-02T14:19:31Z

Landed in 65fe999 🎉

PR-URL: #22921 Refs: #1826 Refs: #22921 (comment) Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Denys Otrishko <shishugi@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>

nodejs-github-bot added the vm Issues and PRs related to the vm subsystem. label Sep 18, 2018

BeniCheni mentioned this pull request Sep 18, 2018

Accept ArrayBuffer (and typed array/data view?) anywhere Buffer is allowed in the API #1826

Closed

addaleax approved these changes Sep 18, 2018

View reviewed changes

targos previously requested changes Sep 18, 2018

View reviewed changes

jasnell approved these changes Oct 1, 2018

View reviewed changes

lundibundi reviewed Oct 3, 2018

View reviewed changes

lundibundi approved these changes Oct 23, 2018

View reviewed changes

refack approved these changes Oct 30, 2018

View reviewed changes

refack added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 30, 2018

BeniCheni added 6 commits October 31, 2018 17:18

vm: update contextify source

8029352

Use ArrayBufferView/IsArrayBufferView in places of Uint8Array/IsUint8Array, in the node_contextify.cc source code. Refs: #22921 (comment)

doc: update vm.md for "cachedData" w. TypedArray|DataView

2010cfd

Update the "cachedData" description with support of TypedArray or DataView support in “new vm.Script” & “vm.compileFunction”.

test: add array buffer view tests in test-vm-cached-data.js

08d177f

test: update tests in test-vm-cached-data.js

bc53ec5

test: update tests in test-vm-cached-data.js

18b86aa

refack closed this Nov 2, 2018

refack added doc Issues and PRs related to the documentations. test Issues and PRs related to the tests. and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Nov 2, 2018

BeniCheni deleted the vm-array-buffer-view branch November 2, 2018 22:33

refack removed their assignment Nov 6, 2018

BridgeAR mentioned this pull request Nov 14, 2018

Release proposal: v11.2 #24350

Merged

MylesBorins added semver-minor PRs that contain new features and should be released in the next minor version. dont-land-on-v8.x labels Nov 27, 2018

BethGriggs mentioned this pull request May 1, 2019

v10.16.0 proposal #27514

Merged

BethGriggs added land-on-v10.x and removed lts-watch-v10.x labels Sep 3, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

vm: update source, related basic & cached data tests for ArrayBuffer #22921

vm: update source, related basic & cached data tests for ArrayBuffer #22921

BeniCheni commented Sep 18, 2018

targos left a comment

targos commented Sep 18, 2018 •

edited

Loading

BeniCheni commented Sep 26, 2018 •

edited

Loading

BeniCheni commented Oct 1, 2018 •

edited

Loading

lundibundi left a comment

BeniCheni commented Oct 3, 2018 •

edited

Loading

lundibundi commented Oct 3, 2018

BeniCheni commented Oct 4, 2018

BeniCheni commented Oct 6, 2018

lundibundi commented Oct 6, 2018 •

edited

Loading

TimothyGu commented Oct 6, 2018

BeniCheni commented Oct 21, 2018 •

edited

Loading

lundibundi Oct 23, 2018

lundibundi commented Oct 23, 2018 •

edited

Loading

BeniCheni commented Oct 23, 2018

BeniCheni commented Oct 30, 2018 •

edited

Loading

refack commented Oct 30, 2018

refack commented Nov 1, 2018

Trott commented Nov 2, 2018

refack commented Nov 2, 2018

vm: update source, related basic & cached data tests for ArrayBuffer #22921

vm: update source, related basic & cached data tests for ArrayBuffer #22921

Conversation

BeniCheni commented Sep 18, 2018

targos left a comment

Choose a reason for hiding this comment

targos commented Sep 18, 2018 • edited Loading

BeniCheni commented Sep 26, 2018 • edited Loading

BeniCheni commented Oct 1, 2018 • edited Loading

lundibundi left a comment

Choose a reason for hiding this comment

BeniCheni commented Oct 3, 2018 • edited Loading

lundibundi commented Oct 3, 2018

BeniCheni commented Oct 4, 2018

BeniCheni commented Oct 6, 2018

lundibundi commented Oct 6, 2018 • edited Loading

TimothyGu commented Oct 6, 2018

BeniCheni commented Oct 21, 2018 • edited Loading

lundibundi Oct 23, 2018

Choose a reason for hiding this comment

lundibundi commented Oct 23, 2018 • edited Loading

BeniCheni commented Oct 23, 2018

BeniCheni commented Oct 30, 2018 • edited Loading

refack commented Oct 30, 2018

refack commented Nov 1, 2018

Trott commented Nov 2, 2018

refack commented Nov 2, 2018

targos commented Sep 18, 2018 •

edited

Loading

BeniCheni commented Sep 26, 2018 •

edited

Loading

BeniCheni commented Oct 1, 2018 •

edited

Loading

BeniCheni commented Oct 3, 2018 •

edited

Loading

lundibundi commented Oct 6, 2018 •

edited

Loading

BeniCheni commented Oct 21, 2018 •

edited

Loading

lundibundi commented Oct 23, 2018 •

edited

Loading

BeniCheni commented Oct 30, 2018 •

edited

Loading