async_hooks: fix leak in AsyncLocalStorage exit#35779
Closed
Uh oh!
There was an error while loading. Please reload this page.
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Qard added confirmed-bug 
vdeturckheim approved these changes Oct 23, 2020
Member
vdeturckheim left a comment
vdeturckheim left a comment There was a problem hiding this comment.
LGTM, thanks a lot @Qard
I feel like _enable() has been added and removed so many times by now ^^
MemberAuthor
Qard commented Oct 23, 2020
Yep. The other option I considered was wrapping the |
MemberAuthor
Qard commented Oct 23, 2020
Whoops...gotta appease that linter. 😅 |
github-actionsbot removed the request-ci 
This comment has been minimized.
This comment has been minimized.
Flarna reviewed Oct 23, 2020
Uh oh!
There was an error while loading. Please reload this page.
Flarna approved these changes Oct 23, 2020
Codecov Report
@@ 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
|
github-actionsbot removed the request-ci This comment has been minimized.
This comment has been minimized.
MemberAuthor
Qard commented Oct 23, 2020
Seems to me like that codecov result probably shouldn't be getting reported for an incomplete test run. 🤔 |
MemberAuthor
CI doesn't want to cooperate. I keep getting seemingly unrelated errors. 🤔 |
This comment has been minimized.
This comment has been minimized.
Trott approved these changes Oct 24, 2020
Trott reviewed Oct 24, 2020
Uh oh!
There was an error while loading. Please reload this page.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Member
puzpuzpuz commented Nov 11, 2020
Created #36079 to follow-up the call stack exceeded issue |
This comment has been minimized.
This comment has been minimized.
Collaborator
nodejs-github-bot commented Nov 11, 2020
Flarna approved these changes Nov 11, 2020
Flarna added the commit-queue github-actionsbot removed the commit-queue 
Contributor
Landed in 39a7f76...06f0d78 |
nodejs-github-bot pushed a commit that referenced this pull request Nov 11, 2020
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]>
MemberAuthor
Qard commented Nov 16, 2020
This should also get backported as any version the AsyncLocalStorage was backported to would also have this issue. |
Flarna added dont-land-on-v10.x and removed review wanted codebytere pushed a commit that referenced this pull request Nov 22, 2020
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]>
Merged
BethGriggs pushed a commit that referenced this pull request Dec 9, 2020
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]>
BethGriggs pushed a commit that referenced this pull request Dec 10, 2020
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]>
Merged
BethGriggs pushed a commit that referenced this pull request Dec 15, 2020
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]>
ruyadorno pushed a commit that referenced this pull request Feb 8, 2021
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]>
Merged
ruyadorno pushed a commit that referenced this pull request Feb 10, 2021
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]>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment
10 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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