Remove strict dependency on Node v6.10.x #1139

swgillespie · 2018-04-09T21:42:43Z

This change makes use of four V8 intrinsics to avoid having to use a
native module to inspect the scope chains of live Function objects. This
unfortunately leads to the limitation of not allowing captures of 'this'
in arrow functions, but that is something we are willing to live with
for now.

Related to #1138 in that we can eliminate the need for our native Node module if we don't allow lexical capture of this in arrow functions. This PR passes all tests on node v6.10.2 that are not this-related. This PR does fail a few tests on node v8.10.0, but I think they are all due to benign differences in the error messages we give for native function capture. For example, here is one such failure:

       '() => handler': closure.spec.js(0,0): captured
         'handler', a function defined at
           '() => os': closure.spec.js(0,0): which captured
             module 'os' which indirectly referenced
      -        function 'cpus': os.js(0,0): which referenced
      -          function 'getCPUs': which could not be serialized because
      -            it was a native code function.
      +        function 'getHostname': which could not be serialized because
      +          it was a native code function.

       Function code:
      -  function getCPUs() { [native code] }
      +  function getHostname() { [native code] }

We'll fail to serialize this closure when we hit the first native code function we see, and apparently node v8.10.0 changed the implementation of os so that the first native code function we try to serialize is getCPUs.

For the changelog:

It is now no longer necessary to use a specific version of Node to run Pulumi programs. The only requirement now is that you must use a version of Node that is verson v6.10.2 or greater when running Pulumi programs.

swgillespie · 2018-04-09T21:43:07Z

This is going to fail a few tests around this until Cyrus's PR goes in.

swgillespie · 2018-04-09T21:53:12Z

#1136

CyrusNajmabadi · 2018-04-09T22:26:41Z

sdk/nodejs/Makefile

-export NODE_PATH:=$(NODE_PATH):./runtime/native/build/Release
-
-ensure::
-	cd runtime/native && ./ensure_node_v8.sh


CyrusNajmabadi · 2018-04-09T22:28:40Z

sdk/nodejs/runtime/closure/native.ts

+// the latter function returns the i'th entry in a function's scope chain, given a function and
+// index i.
+const getFunctionScopeDetails: (func: Function, index: number) => any[] =
+    new Function("func", "index", "return %GetFunctionScopeDetails(func, index);") as any;


instead of a variable that is assigned a lambda... can you just make this a normal function? That seems like it would read a lot nicer... (or is it necessary for this to be a lambda to work properly?)

Ignore this.

CyrusNajmabadi · 2018-04-09T22:28:46Z

sdk/nodejs/runtime/closure/native.ts

+const getFunctionScopeDetails: (func: Function, index: number) => any[] =
+    new Function("func", "index", "return %GetFunctionScopeDetails(func, index);") as any;
+const getFunctionScopeCount: (func: Function) => number =
+    new Function("func", "return %GetFunctionScopeCount(func);") as any;


CyrusNajmabadi · 2018-04-09T22:29:51Z

sdk/nodejs/runtime/closure/native.ts

+    }
+
+    const pos = getSourcePosition(func);
+    const location = script.locationFromPosition(pos);


since the tests set all line/cols to 0, can you manually test this and validate that everything is the correct 1-based numbers in the output?

Here's an error using node v8.10.0 that you get when you try to capture os:

$ node --version v8.10.0 [0 Seans-MacBook-Pro ~/go/src/github.com/pulumi/scratch/stack] $ pulumi update Performing changes: * pulumi:pulumi:Stack: (same) [urn=urn:pulumi:test::stack::pulumi:pulumi:Stack::stack-test] error: Error serializing '() => os': index.js(5,54) '() => os': index.js(5,54): captured module 'os' which indirectly referenced function 'cpus': os.js(85,13): which referenced function 'getCPUs': which could not be serialized because it was a native code function. Function code: function getCPUs() { [native code] } Capturing modules can sometimes cause problems. Consider using import('os') or require('os') inside '() => os': index.js(5,54) info: no changes required: 1 resource unchanged error: an unhandled error occurred: Program exited with non-zero exit code: 1

The positions in index.js look correct to me.

ellismg · 2018-04-09T22:36:17Z

sdk/nodejs/runtime/closure/native.ts

+// As a side-effect of importing this file, we must enable the --allow-natives-syntax V8
+// flag. This is because we are using V8 intrinsics in order to implement this module.
+import * as v8 from "v8";
+v8.setFlagsFromString("--allow-natives-syntax");


Given that the language host is going to launch node directly, can we just pass this to node itself when we launch it, instead of trying to set it at run-time?

We can, but I found that to be pretty painful for dev scenarios. We use istanbul wrapping mocha to run our tests, and istanbul doesn't give us direct control over the Node command line(1). I ended up having to hack up the Makefiles to get it to work last time I went down this route.

I'm happy to do that if that's better, but this Node API is very stable and does effectively the same thing.

(1) Mocha doesn't either, but its command-line driver is intelligent enough to pass --allow-natives-syntax onto node.

I prefer that we set it in code like this, since it can be done in TypeScript (no native code), and guarantees that it's set anytime the Pulumi runtime is loaded. Just seems less error prone.

BTW, what happens if we fail to set this? Closure serialization throws? Silently does the wrong thing?

P. S. I did find it confusing that this is still called native.ts. I kept looking at it, expecting C++ code, and then getting confused by the presence of JavaScript. Probably just historical, but maybe v8.ts?

BTW, what happens if we fail to set this? Closure serialization throws? Silently does the wrong thing?

You'll get an immediate crash on startup.

> new Function("%AbortJS") SyntaxError: Unexpected token %

Probably just historical, but maybe v8.ts?

Sure, I can do that. It collides with the builtin node module v8, but we shouldn't be using that anywhere other than this file anyway, so seems fine to me.

CyrusNajmabadi · 2018-04-09T22:37:23Z

sdk/nodejs/runtime/closure/native.ts

+export function lookupCapturedVariableValue(func: Function, freeVariable: string, throwOnFailure: boolean): any {
+    // The implementation of this function is now very straightforward since the intrinsics do all of the
+    // difficult work.
+    const scopes = getScopesForFunction(func);


instead of getting the scopes, and then looking up in teh scopes... coudl we not just walk the scopes, stopping whne we find the thing we're looking for?

Yep, we can do that - it's probably more efficient, too, since V8 allocates a ScopeIterator for each call to GetFunctionScopeDetails.

This change makes use of four V8 intrinsics to avoid having to use a native module to inspect the scope chains of live Function objects. This unfortunately leads to the limitation of not allowing captures of 'this' in arrow functions, but that is something we are willing to live with for now.

…odule

joeduffy

🎉 🍾 💯

joeduffy · 2018-04-09T23:48:33Z

sdk/nodejs/Makefile


 lint::
 	$(GOMETALINTER) cmd/pulumi-language-nodejs/main.go | sort ; exit "$${PIPESTATUS[0]}"
 	tslint -c tslint.json -p tsconfig.json

 build::
 	go install -ldflags "-X github.com/pulumi/pulumi/pkg/version.Version=${VERSION}" ${LANGUAGE_HOST}
-	cd runtime/native && node-gyp configure


I expected to also see a boatload of red C++ code in this change? 😀

coming up in the next commit! 😆

joeduffy · 2018-04-09T23:52:27Z

sdk/nodejs/runtime/closure/native.ts

+// As a side-effect of importing this file, we must enable the --allow-natives-syntax V8
+// flag. This is because we are using V8 intrinsics in order to implement this module.
+import * as v8 from "v8";
+v8.setFlagsFromString("--allow-natives-syntax");


I prefer that we set it in code like this, since it can be done in TypeScript (no native code), and guarantees that it's set anytime the Pulumi runtime is loaded. Just seems less error prone.

BTW, what happens if we fail to set this? Closure serialization throws? Silently does the wrong thing?

joeduffy · 2018-04-09T23:53:46Z

sdk/nodejs/runtime/closure/native.ts

+// As a side-effect of importing this file, we must enable the --allow-natives-syntax V8
+// flag. This is because we are using V8 intrinsics in order to implement this module.
+import * as v8 from "v8";
+v8.setFlagsFromString("--allow-natives-syntax");


P. S. I did find it confusing that this is still called native.ts. I kept looking at it, expecting C++ code, and then getting confused by the presence of JavaScript. Probably just historical, but maybe v8.ts?

joeduffy · 2018-04-09T23:55:53Z

sdk/nodejs/runtime/closure/native.ts

+    // The use of the Function constructor here and elsewhere in this file is because
+    // because V8 intrinsics are not valid JavaScript identifiers; they all begin with '%',
+    // which means that the TypeScript compiler issues errors for them.
+    new Function("func", "return %FunctionGetScript(func);") as any;


If there is any public documentation on %FunctionGetScript -- and any other such APIs below -- do you mind also linking to it from within this file? It seems hard to find good information about them!

there's no documentation whatsoever! 😈 The only docs are the source of v8 itself.

Honestly, we are probably the experts on this topic now...

joeduffy · 2018-04-09T23:57:10Z

sdk/nodejs/runtime/closure/native.ts

+const getSourcePosition: (func: Function) => V8SourcePosition =
+    new Function("func", "return %FunctionGetScriptSourcePosition(func);") as any;
+
+interface V8SourcePosition {}


Maybe document these types? I gather that V8SourcePosition is simply an object with identity that can be used to retrieve an associated V8SourceLocation? (Perhaps it has additional state we're simply choosing not to project at the moment?) Another case where linking to docs would be good.

Will do, and yeah, that intuition is correct. No docs, but whatever V8SourcePosition is needs to be passed verbatim to locationFromPosition.

Worth noting, though, every interface in here is a subset of the full object; I only encoded in the type system the fields that we intend to use here.

…8.ts

swgillespie · 2018-04-10T17:03:55Z

Everything's green so I'll go ahead and merge this per the feedback. Let me know if I should make any changes.

CyrusNajmabadi

👍

swgillespie requested review from joeduffy, CyrusNajmabadi and ellismg April 9, 2018 21:42

joeduffy added the in progress label Apr 9, 2018

swgillespie added area/sdks Pulumi language SDKs customer/feedback labels Apr 9, 2018

CyrusNajmabadi reviewed Apr 9, 2018

View reviewed changes

ellismg reviewed Apr 9, 2018

View reviewed changes

CyrusNajmabadi reviewed Apr 9, 2018

View reviewed changes

swgillespie added 3 commits April 9, 2018 16:09

Remove native module build and restore from the Makefile

02bd0a1

CR feedback: Be a little more efficient when scanning the scope chain

f443fb0

swgillespie force-pushed the swgillespie/pure-javascript-scopechain branch from 0b976bf to f443fb0 Compare April 9, 2018 23:17

Nuke everything related to custom Node versions and the native Node m…

538f7d7

…odule

joeduffy approved these changes Apr 9, 2018

View reviewed changes

CR feedback: rename native.ts -> v8.ts, document some interfaces in v…

0cd309c

…8.ts

swgillespie merged commit 682f908 into master Apr 10, 2018

swgillespie deleted the swgillespie/pure-javascript-scopechain branch April 10, 2018 17:04

joeduffy removed the in progress label Apr 10, 2018

swgillespie added the impact/changelog label Apr 10, 2018

CyrusNajmabadi reviewed Apr 10, 2018

View reviewed changes

lindydonna added the kind/feature label Apr 14, 2018

lindydonna changed the title ~~Implement scope chain free variable lookup in pure TypeScript~~ Remove strict dependency on Node v6.10.x Apr 14, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove strict dependency on Node v6.10.x #1139

Remove strict dependency on Node v6.10.x #1139

swgillespie commented Apr 9, 2018 •

edited

Loading

swgillespie commented Apr 9, 2018

swgillespie commented Apr 9, 2018

CyrusNajmabadi Apr 9, 2018

CyrusNajmabadi Apr 9, 2018

CyrusNajmabadi Apr 9, 2018

CyrusNajmabadi Apr 9, 2018

CyrusNajmabadi Apr 9, 2018

swgillespie Apr 9, 2018

swgillespie Apr 9, 2018

ellismg Apr 9, 2018

swgillespie Apr 9, 2018 •

edited

Loading

joeduffy Apr 9, 2018

joeduffy Apr 9, 2018

swgillespie Apr 10, 2018

CyrusNajmabadi Apr 9, 2018

swgillespie Apr 9, 2018

joeduffy left a comment

joeduffy Apr 9, 2018

swgillespie Apr 10, 2018

joeduffy Apr 9, 2018

joeduffy Apr 9, 2018

joeduffy Apr 9, 2018

swgillespie Apr 10, 2018 •

edited

Loading

joeduffy Apr 9, 2018

swgillespie Apr 10, 2018

swgillespie Apr 10, 2018

swgillespie commented Apr 10, 2018

CyrusNajmabadi left a comment

Remove strict dependency on Node v6.10.x #1139

Remove strict dependency on Node v6.10.x #1139

Conversation

swgillespie commented Apr 9, 2018 • edited Loading

swgillespie commented Apr 9, 2018

swgillespie commented Apr 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swgillespie Apr 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joeduffy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swgillespie Apr 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swgillespie commented Apr 10, 2018

CyrusNajmabadi left a comment

Choose a reason for hiding this comment

swgillespie commented Apr 9, 2018 •

edited

Loading

swgillespie Apr 9, 2018 •

edited

Loading

swgillespie Apr 10, 2018 •

edited

Loading