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

Revert "Re-inject native Node modules" #5009

Merged
merged 9 commits into from
Dec 4, 2017

Conversation

mjesun
Copy link
Contributor

@mjesun mjesun commented Dec 4, 2017

This PR reverts #4970. I've been thinking for some days about reverting this, and I've finally decided to go ahead.

The initial reason to fully sandbox native modules was to eliminate memory leaks. While it fixed some of the leaks (memory consumption in the own Jest repository went down by approximately 10%), this PR introduced many other issues which I've been progressively fixing:

  • It made the process object not to inherit from EventEmitter anymore (now that events is re-injected, a new instance is created, but the process object is an instance of the parent EventEmitter, not the re-injected one).

  • We needed to create a switch statement because some of the modules cannot be re-injected (e.g. re-injecting async_hooks makes V8 crash); and some of them are just re-created (e.g. os module).

  • We also had to nullify some references to be able to get rid of some additional memory leaks. This created other issues, like the one we tried fixing in Change the way setting up the framework works #4976.

  • This PR still did not solve other additional issues, like the ./child test from jest-worker not working on parallel execution.

On top of that, when applying the newest version (21.3.0-beta.11) to Yarn, we discovered some additional issues, which are derived from the lack of Node's initialization code (the first file executed in V8, internal/bootstrap_node, is not injected, so things like internal/process/next_tick are not properly initialized, making the tests fail).

In theory, injecting internal/bootstrap_node would fix these issues, but it might led to new ones. And since the initial problem was not to sandbox modules, but to reduce memory consumption, and we haven't really achieved that, I think the best for now is to revert. Maybe someone can pick the work done here and try pushing a bit forward.

@mjesun mjesun requested review from cpojer and thymikee December 4, 2017 11:30
@cpojer
Copy link
Member

cpojer commented Dec 4, 2017

Hm, I was really excited about this change but it seems like it creates a lot of issues in some cases. I thought that considering it won't completely fix all memory issues it is still super valuable because it will sandbox everything properly. I guess the summary of your revert is that we tried to create a perfect sandbox inside of a vm context for node but the only way to really do it, without copying individual internal source files of node into Jest, is by forking a node process which is really slow. I'll land this revert and release a new beta but it would be great if we could make Jest more resilient to most common memory leaks regardless, and if we could sandbox the things that need sandboxing separately.

@cpojer cpojer merged commit 6d0c0f0 into master Dec 4, 2017
@mjesun mjesun deleted the revert-4970-re-inject-native-modules branch December 6, 2017 11:39
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants