Fix destruction of builtin node globals#15636
Conversation
✅ Deploy Preview for jestjs ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify project configuration. |
eyalroth
changed the title
| Object.getOwnPropertyNames(global) as GlobalProperties, | ||
| ); | ||
| for (const [nodeGlobalsKey, descriptor] of nodeGlobals) { | ||
| protectProperties(global[nodeGlobalsKey]); |
There was a problem hiding this comment.
This seems to fix #15635 if we change this line to:
| protectProperties(global[nodeGlobalsKey]); | |
| protectProperties(globalThis[nodeGlobalsKey]); |
There was a problem hiding this comment.
@lencioni Yes that was actually my original code which I tested on. global is the variable from line 95 which at this point doesn't even have nodeGlobalsKey, and so protectProperties won't do a thing.
I think what happens is that in my files I was editing the compiled file (index.js) to avoid waiting for complication every time, and when I moved the code to the source file (index.ts) I accidentally changed the variable (AI auto complete? that would be a conspiracy).
In any case, it worries me that the CI tests don't pick on this, because the unit tests that I added definitely fail on this:
$ yarn test -- jest-environment-node
PASS packages/jest-environment-node/src/__tests__/node_environment.test.ts
FAIL packages/jest-environment-node/src/__tests__/node_environment_2.test.ts
● NodeEnvironment 2 › dispatch event
TypeError: Value of "this" must be of type EventTarget
at Object.dispatchEvent (packages/jest-environment-node/src/__tests__/node_environment_2.test.ts:10:23)
Test Suites: 1 failed, 1 passed, 2 total
Tests: 1 failed, 7 passed, 8 total
Snapshots: 0 total
Time: 0.331 s, estimated 1 s
Ran all test suites matching jest-environment-node.
(and also yarn test locally)
In any case, I will amend the commit.
eyalroth
force-pushed
the
15632-fix-destruction-node-globals
branch
from
c3cee80 to
7b33efa
Compare
|
PR updated with amend + force push (see comment conversation). @cpojer FYI it looks like there are issues with the CI tests:
|
cpojer
left a comment
cpojer
left a comment
There was a problem hiding this comment.
Wow, thanks everyone for being so on top of things!
…anup on env teardown (to reduce memory leaks)
|
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. |
3 participants
Summary
Fixes #15632 that was introduced in #15215.
The PR to mitigate memory leaks accidentally deleted some of the builtin node globals.
This PR fixes it by protecting all of them from deletion.
Test plan
New tests were added that rely on the reproduction project from #15632.
Looks like one CI test (
e2e/__tests__/circusConcurrent.test.ts) fails, but from the output it seems to be flaky?I see previous builds where it fails several times but succeeds on the last (3rd) attempt.