async_hooks: fix leak in AsyncLocalStorage exit #35779

Qard · 2020-10-23T18:18:35Z

If exit is called and then run or enterWith are called within the exit function, the als instance 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.

This PR fixes a memory leak wherein an AsyncLocalStorage instance could be repeatedly inserted to the storageList array which keeps track of active instances to call als._propagate(...) on in the shared async_hooks instance.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

vdeturckheim

LGTM, thanks a lot @Qard
I feel like _enable() has been added and removed so many times by now ^^

Qard · 2020-10-23T18:24:22Z

Yep. The other option I considered was wrapping the exit function in another AsyncResource without the storage slot, which I believe we also had at one point but removed due to people thinking the overhead of another AsyncResource was unnecessary. 😅

Qard · 2020-10-23T18:26:29Z

Whoops...gotta appease that linter. 😅

mhdawson

LGTM

test/parallel/test-async-local-storage-exit-does-not-leak.js

puzpuzpuz

Nice catch!

codecov-io · 2020-10-23T19:25:24Z

Codecov Report

Merging #35779 into master will increase coverage by 0.22%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master   #35779      +/-   ##
==========================================
+ Coverage   87.92%   88.14%   +0.22%     
==========================================
  Files         477      478       +1     
  Lines      113090   113536     +446     
  Branches    24630    25553     +923     
==========================================
+ Hits        99433   100080     +647     
+ Misses       7952     7698     -254     
- Partials     5705     5758      +53

Impacted Files	Coverage Δ
lib/async_hooks.js	`100.00% <100.00%> (ø)`
src/connect_wrap.h	`25.00% <0.00%> (-75.00%)`	⬇️
lib/internal/repl/history.js	`88.16% <0.00%> (-4.15%)`	⬇️
src/api/utils.cc	`28.57% <0.00%> (-2.86%)`	⬇️
lib/_http_incoming.js	`97.47% <0.00%> (-2.53%)`	⬇️
lib/internal/modules/esm/get_source.js	`83.33% <0.00%> (-2.39%)`	⬇️
lib/internal/dns/utils.js	`98.44% <0.00%> (-1.56%)`	⬇️
lib/internal/modules/esm/loader.js	`86.22% <0.00%> (-1.19%)`	⬇️
src/inspector_profiler.cc	`76.17% <0.00%> (-1.09%)`	⬇️
src/node_worker.cc	`76.56% <0.00%> (-0.96%)`	⬇️
... and 108 more

Qard · 2020-10-23T21:00:23Z

Seems to me like that codecov result probably shouldn't be getting reported for an incomplete test run. 🤔

Qard · 2020-10-23T21:51:22Z

CI doesn't want to cooperate. I keep getting seemingly unrelated errors. 🤔

test/parallel/test-async-local-storage-exit-does-not-leak.js

puzpuzpuz · 2020-11-11T06:42:58Z

Created #36079 to follow-up the call stack exceeded issue

nodejs-github-bot · 2020-11-11T08:46:38Z

CI: https://ci.nodejs.org/job/node-test-pull-request/34315/

github-actions · 2020-11-11T11:32:56Z

Landed in 39a7f76...06f0d78

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. PR-URL: #35779 Reviewed-By: Vladimir de Turckheim <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Gerhard Stöbich <[email protected]> Reviewed-By: Andrey Pechkurov <[email protected]> Reviewed-By: Rich Trott <[email protected]>

Qard · 2020-11-16T18:35:03Z

This should also get backported as any version the AsyncLocalStorage was backported to would also have this issue.

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. PR-URL: #35779 Reviewed-By: Vladimir de Turckheim <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Gerhard Stöbich <[email protected]> Reviewed-By: Andrey Pechkurov <[email protected]> Reviewed-By: Rich Trott <[email protected]>

Qard added confirmed-bug Issues with confirmed bugs. async_hooks Issues and PRs related to the async hooks subsystem. review wanted PRs that need reviews. request-ci Add this label to start a Jenkins CI on a PR. labels Oct 23, 2020

Qard requested review from vdeturckheim, Flarna and puzpuzpuz October 23, 2020 18:18

Qard force-pushed the fix-als-exit branch from 7543153 to 2d7f986 Compare October 23, 2020 18:19

vdeturckheim approved these changes Oct 23, 2020

View reviewed changes

Qard force-pushed the fix-als-exit branch from 2d7f986 to 43fb5bb Compare October 23, 2020 18:26

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 23, 2020

This comment has been minimized.

Sign in to view

mhdawson approved these changes Oct 23, 2020

View reviewed changes

Flarna reviewed Oct 23, 2020

View reviewed changes

test/parallel/test-async-local-storage-exit-does-not-leak.js Show resolved Hide resolved

Flarna approved these changes Oct 23, 2020

View reviewed changes

puzpuzpuz approved these changes Oct 23, 2020

View reviewed changes

rochdev mentioned this pull request Oct 23, 2020

fix memory leak in the async_local_storage scope manager DataDog/dd-trace-js#1134

Merged

Qard added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 23, 2020

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 23, 2020

This comment has been minimized.

Sign in to view

Trott approved these changes Oct 24, 2020

View reviewed changes

Trott reviewed Oct 24, 2020

View reviewed changes

test/parallel/test-async-local-storage-exit-does-not-leak.js Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

puzpuzpuz mentioned this pull request Nov 11, 2020

async_hooks: async hook stack assertion shadows "Maximum call stack size exceeded" #36079

Closed

This comment has been minimized.

Sign in to view

Flarna approved these changes Nov 11, 2020

View reviewed changes

Flarna added the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 11, 2020

github-actions bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 11, 2020

github-actions bot closed this Nov 11, 2020

Qard deleted the fix-als-exit branch November 11, 2020 23:19

Flarna added dont-land-on-v10.x and removed review wanted PRs that need reviews. labels Nov 16, 2020

codebytere mentioned this pull request Nov 22, 2020

v15.3.0 proposal #36232

Merged

BethGriggs mentioned this pull request Dec 10, 2020

v14.15.2 proposal #36476

Merged

BethGriggs removed the lts-watch-v14.x label Dec 16, 2020

ruyadorno mentioned this pull request Feb 9, 2021

v12.20.2 proposal #37286

Merged

richardlau added backported-to-v12.x and removed lts-watch-v12.x labels Jul 31, 2021

vongruenigen mentioned this pull request Jan 21, 2024

Severe performance degradation with long running processes adam-cowley/nest-neo4j#20

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

async_hooks: fix leak in AsyncLocalStorage exit #35779

async_hooks: fix leak in AsyncLocalStorage exit #35779

Qard commented Oct 23, 2020

vdeturckheim left a comment

Qard commented Oct 23, 2020

Qard commented Oct 23, 2020

This comment has been minimized.

mhdawson left a comment

puzpuzpuz left a comment

codecov-io commented Oct 23, 2020 •

edited

Loading

This comment has been minimized.

Qard commented Oct 23, 2020

Qard commented Oct 23, 2020 •

edited

Loading

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

puzpuzpuz commented Nov 11, 2020

This comment has been minimized.

nodejs-github-bot commented Nov 11, 2020

github-actions bot commented Nov 11, 2020

Qard commented Nov 16, 2020

async_hooks: fix leak in AsyncLocalStorage exit #35779

async_hooks: fix leak in AsyncLocalStorage exit #35779

Conversation

Qard commented Oct 23, 2020

Checklist

vdeturckheim left a comment

Choose a reason for hiding this comment

Qard commented Oct 23, 2020

Qard commented Oct 23, 2020

This comment has been minimized.

mhdawson left a comment

Choose a reason for hiding this comment

puzpuzpuz left a comment

Choose a reason for hiding this comment

codecov-io commented Oct 23, 2020 • edited Loading

Codecov Report

This comment has been minimized.

Qard commented Oct 23, 2020

Qard commented Oct 23, 2020 • edited Loading

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

puzpuzpuz commented Nov 11, 2020

This comment has been minimized.

nodejs-github-bot commented Nov 11, 2020

github-actions bot commented Nov 11, 2020

Qard commented Nov 16, 2020

codecov-io commented Oct 23, 2020 •

edited

Loading

Qard commented Oct 23, 2020 •

edited

Loading