Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upasync_hooks: fix leak in AsyncLocalStorage exit #35779
Conversation
|
LGTM, thanks a lot @Qard |
|
Yep. The other option I considered was wrapping the |
If exit is called and then run or enterWith are called within the exit function, the als instace should not be added to the storageList additional times. The correct behaviour is to remove the instance from the storageList before executing the exit handler and then to restore it after.
|
Whoops...gotta appease that linter. |
|
LGTM |
|
Nice catch! |
Codecov Report
@@ Coverage Diff @@
## master #35779 +/- ##
==========================================
- Coverage 87.92% 87.91% -0.01%
==========================================
Files 477 477
Lines 113090 113088 -2
Branches 24630 24628 -2
==========================================
- Hits 99433 99424 -9
- Misses 7952 7954 +2
- Partials 5705 5710 +5
|
|
Seems to me like that codecov result probably shouldn't be getting reported for an incomplete test run. |
|
CI doesn't want to cooperate. I keep getting seemingly unrelated errors. |
This comment has been hidden.
This comment has been hidden.
This comment has been hidden.
This comment has been hidden.
Co-authored-by: Rich Trott <rtrott@gmail.com>
| // lib/async_hooks.js when exit(...) is called, therefore when the nested runs | ||
| // are called there should be no copy of the als in the storageList to run the | ||
| // _propagate method on. | ||
| als._propagate = common.mustNotCall('_propagate() should not be called'); |
Flarna
Oct 24, 2020
•
Member
This is quite sensitive, e.g. someone renames _propagate and this test still passes but is worthless.
What about
Suggested change
als._propagate = common.mustNotCall('_propagate() should not be called');
const fName = "_propagate";
assert.ok(typeof(als[fName]) === "function");
als[fName] = common.mustNotCall('_propagate() should not be called');
This is quite sensitive, e.g. someone renames _propagate and this test still passes but is worthless.
What about
| als._propagate = common.mustNotCall('_propagate() should not be called'); | |
| const fName = "_propagate"; | |
| assert.ok(typeof(als[fName]) === "function"); | |
| als[fName] = common.mustNotCall('_propagate() should not be called'); |
If
exitis called and thenrunorenterWithare called within theexitfunction, the als instance should not be added to thestorageListadditional times. The correct behaviour is to remove the instance from thestorageListbefore executing theexithandler and then to restore it after.This PR fixes a memory leak wherein an
AsyncLocalStorageinstance could be repeatedly inserted to thestorageListarray which keeps track of active instances to callals._propagate(...)on in the shared async_hooks instance.Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes