Repository navigation
node_contextify tracking issue #6283
Description
The implementation of the vm module has some limitation that results in non-intuitive behaviour. There are already a few bugs open for this (see list below). At this point I do not think there are incremental fixes that can solve the issues with the vm module. I suspect that a revamp of the vm module might be needed, with some API help from the V8 team, to fix these issues properly.
Existing open issues:
- vm: references to context inside objects are not === the original context #855: references to context inside objects are not === the original context
- vm: accessor properties get converted to data properties inside the vm #2734: accessor properties get converted to data properties
- sandbox inheritied properties flattened after vm.runInNewContext #5350: inherited properties get flattened
- Proxy passed as vm context hides built-in properties. #6158: Proxy passed as vm context hides built-in properties
- VM: weird behaviour in runInContext/runInThisContext #5344: Strict mode global property update should not succeed
- VM context object silently fails on Object.defineProperty #5679: Object.defineProperty
- [code comment] Async updates in context aren't reflected in the sandbox object.
- Function redefinition in vm.runInContext #548, fixed upstream in V8 (September 2016)
- vm misses const definitions after recursive loads (only in v0.11.15) node-v0.x-archive#9084 (but it may be a dupe of VM top level vars are lost after a few iterations #769 which was fixed in recent V8)
- deleting property in vm context has no effect #6287 deleting a property in a context has no effect
- Built-in properties cannot be accessed using Reflect, when Proxy is passed as vm context #7458 Built-in properties cannot be accessed using Reflect, when Proxy is passed as vm context
- vm: strict mode ReferenceError with assignment to context property #12300 vm: strict mode ReferenceError with assignment to context property
- probably more that I am not aware of... (please add in comments below and I will update the issue)
I am creating this issue to make it easier to keep track of these issues, and possible solutions.
/cc @nodejs/v8 @domenic.
Activity
domenic commentedon Apr 19, 2016
fhinkel commentedon Jul 2, 2016
Proof of Concept: intercept defineProperty()
bmeck commentedon Sep 29, 2016
The problem is API design that I can tell. We "enhance" and return the
sandboxobject passed in tovm.createContext()but the behavior of thev8::Context::Global()in v8 cannot delegate fully with behaviors back to the sandbox (likeObject.defineProperty). We should really return a newProxyfromvm.createContextthat can propagate into the VMGlobal(). This is a breaking change but this relates to most of this list.fhinkel commentedon Sep 29, 2016
We discussed the proxy idea in #7820.
We made API changes in V8. Fixing the issues here should be possible as soon as we pull in a newer V8 version.
bmeck commentedon Sep 29, 2016
@fhinkel that was describing a Proxy inside of the Context as the Global proxy, I was thinking of a Proxy outside of the v8::Context and using something like
newRemoteContext. The use case described by @domenic to my knowledge was around needing to generate theGlobal()reference (that is===) prior to creating a full context. Looking at the code as well, it seems that Node should be usingFromSnapshotwhich it currently does not. If possible I would like to move away from interceptors if possible.dead-claudia commentedon Oct 3, 2016
BTW the box for #5344 should probably now be checked, since that issue is now closed.
fhinkel commentedon Dec 11, 2016
#10223
cpojer commentedon Dec 15, 2016
I'm really excited about improvements to vm context. Jest, the JavaScript testing framework, heavily uses the vm module to isolate tests. Is there any way we can make sure that Jest keeps working or that we are aware of potential breaking changes? Is it at all possible for you to include
yarn test(oryarn test -ito run everything within a single process) inside of the Jest repo to the test plans for changes here? Jest is used for testing by a ton of companies, so it may make sense to ensure we don't break anything here and that we are aware of changes that are coming.It may actually be a good real-world test for you, too. I'm happy to talk more about the architecture but in essence Jest parallelizes across worker processes and has a custom node-like (or jsdom) env in a vm context that also comes with a powerful custom require implementation that can be used for module-boundary mocking.
I also had thoughts around
vm.Script. It has the cached data feature via theproduceCachedDataandcachedDatafields. I had hopes that this would speed up repeated script execution which could be pretty significant for Jest but was surprised there was no performance benefit of using this at all across thousands of test files and tens of thousands of modules at FB. Besides the API of this feature, which is really odd, is there any way this feature can be changed to be more efficient? I kind of don't really get why v8 has to validate the cached data as that also requires to store both the original source and the cached data. This may be topic for a separate issue but I just wanted to bring it up.Context:
jest-environment-nodethat creates a context.jest-runtimethat implements require.fhinkel commentedon Dec 15, 2016
Good idea to use Jest tests as test cases for vm changes. Anything we break, we can add as regression tests (and fix it of course). The vm module could certainly have a better test coverage.
@bnoordhuis recently ran some benchmarks with and without vm. Slowdown is pretty dramatic. Definitely worth looking into how to speed it up.
bmeck commentedon Dec 15, 2016
@cpojer in order to see speed gains vs the IO you need to implement a large cache store, doing it per file might actually slow things down. I can walk you through it sometime if you want.
cpojer commentedon Dec 15, 2016
A large cache store of what? In the third link I shared above you can see how we share
vm.Scriptinstances during a single run; as long as the file doesn't change, so there isn't any IO involved.bmeck commentedon Dec 15, 2016
@cpojer i see no call w/ produceCachedData and no file in which those results are stored in the source https://github.com/facebook/jest/search?utf8=%E2%9C%93&q=produceCachedData&type=Issues . Even if this is implemented the wrapper you show has a per file cache of the source transform, and it can be expensive to load in cache files off disk ad-hoc vs as a large bundle of cachedData. This is somewhat off topic and might be better done offline.
10 remaining items
fhinkel commentedon Oct 23, 2017
@ofrobots The remaining 4 issues are not related. Should we close this tracking issue?
src: use new V8 API in vm module
ofrobots commentedon Oct 27, 2017
Agree. Closing.
src: use new V8 API in vm module