src: release context frame in AsyncWrap::EmitDestroy#61995
src: release context frame in AsyncWrap::EmitDestroy#61995nodejs-github-bot merged 4 commits intonodejs:mainfrom
Conversation
Release the async context frame in AsyncWrap::EmitDestroy to allow gc to collect it. This is in special relevant for reused resources like HTTPParser otherwise they might keep ALS stores alive.
391ae5a to
bdd2d38
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #61995 +/- ##
==========================================
- Coverage 89.77% 89.64% -0.13%
==========================================
Files 674 676 +2
Lines 205705 206252 +547
Branches 39449 39511 +62
==========================================
+ Hits 184670 184897 +227
- Misses 13280 13476 +196
- Partials 7755 7879 +124
🚀 New features to boost your workflow:
|
| HandleScope handle_scope(env()->isolate()); | ||
| USE(object()->Set(env()->context(), env()->resource_symbol(), object())); | ||
| } | ||
| context_frame_.Reset(); |
There was a problem hiding this comment.
Not for this PR, but do you know how it comes this is a v8::Global to begin with and not an internal field on object()?
There was a problem hiding this comment.
No particular reason, I guess. Probably could be an internal field. Just needed to have a context frame reference somewhere related to an AsyncWrap so it could do the restore between the before and after. It was just easier to store directly on the AsyncWrap since the point it captures at was already on the native side.
Co-authored-by: Anna Henningsen <github@addaleax.net>
There was a problem hiding this comment.
Sorry, just saw your comments on the DCHECK (hidden because of the resolved comment) – that's exactly why the DCHECK should be in place, we should not be able to MakeCallback() when there isn't a corresponding context frame to be entered.
I can try to find some time to dig into those failures.
This comment was marked as outdated.
This comment was marked as outdated.
|
Yeah, the only error I'm getting is this one: That's the test from #37958, which was added to complete coverage and intentionally uses internal APIs. We could skip the DCHECK for now, but it does catch a valid issue here I'd say. |
|
Setting The failing test shows actually a race in HTTP:
Adding But well, thats an unrelated issue for an http expert. Do you want me to remove the |
No. We should have something that tells us if the behavior is broken that way. c458399 only fixed the test by breaking the I'm kind of leaning towards something like #ifdef DEBUG
if (
context_frame_.IsEmpty() ||
context_frame_.Get(env()->isolate())->IsUndefined()
) {
ProcessEmitWarning(env(), "MakeCallback() called without context frame");
}
#endifwith the goal of eventually turning it into a regular |
|
I don't think emitting a warning in case
The ALS implementation is handling We could move towards setting some What about adding following and reverting c458399 ? |
Yeah, that would work then 👍 |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Commit Queue failed- Loading data for nodejs/node/pull/61995 ✔ Done loading data for nodejs/node/pull/61995 ----------------------------------- PR info ------------------------------------ Title src: release context frame in AsyncWrap::EmitDestroy (#61995) ⚠ Could not retrieve the email or name of the PR author's from user's GitHub profile! Branch Flarna:als-http-parser-leak -> nodejs:main Labels c++, author ready, needs-ci, commit-queue-squash, dont-land-on-v20.x, async_local_storage Commits 4 - src: release context frame in AsyncWrap::EmitDestroy - Add DCHECK to ensure context_frame is not emtpy in MakeCallback - use undefined to avoid empty global - set empty context_frame_, issue a process warning Committers 2 - Gerhard Stöbich <18708370+Flarna@users.noreply.github.com> - GitHub <noreply@github.com> PR-URL: https://github.com/nodejs/node/pull/61995 Fixes: https://github.com/nodejs/node/issues/61882 Refs: https://github.com/nodejs/node/pull/48528 Reviewed-By: Anna Henningsen <anna@addaleax.net> ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/61995 Fixes: https://github.com/nodejs/node/issues/61882 Refs: https://github.com/nodejs/node/pull/48528 Reviewed-By: Anna Henningsen <anna@addaleax.net> -------------------------------------------------------------------------------- ℹ This PR was created on Thu, 26 Feb 2026 00:07:32 GMT ✔ Approvals: 1 ✔ - Anna Henningsen (@addaleax): https://github.com/nodejs/node/pull/61995#pullrequestreview-3879134622 ✘ This PR needs to wait 42 more hours to land (or 0 minutes if there is one more approval) ✔ Last GitHub CI successful ℹ Last Full PR CI on 2026-03-03T05:16:27Z: https://ci.nodejs.org/job/node-test-pull-request/71538/ - Querying data for job/node-test-pull-request/71538/ ✔ Build data downloaded ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ Aborted `git node land` session in /home/runner/work/node/node/.ncuhttps://github.com/nodejs/node/actions/runs/22611759900 |
Qard
left a comment
There was a problem hiding this comment.
LGTM. As for the note about v8::Global versus internal field: there's no particular reason it is the way it is. Just seemed more straightforward at the time since the frame value is initially captured from the native side. Simply adding it to the AsyncWrap rather than the wrapped JS object itself seemed the most straightforward thing to do at the time.
| HandleScope handle_scope(env()->isolate()); | ||
| USE(object()->Set(env()->context(), env()->resource_symbol(), object())); | ||
| } | ||
| context_frame_.Reset(); |
There was a problem hiding this comment.
No particular reason, I guess. Probably could be an internal field. Just needed to have a context frame reference somewhere related to an AsyncWrap so it could do the restore between the before and after. It was just easier to store directly on the AsyncWrap since the point it captures at was already on the native side.
|
Landed in 4eac937 |
Release the async context frame in AsyncWrap::EmitDestroy to allow gc to collect it. This is in special relevant for reused resources like HTTPParser otherwise they might keep ALS stores alive. PR-URL: #61995 Fixes: #61882 Refs: #48528 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
This MR contains the following updates: | Package | Update | Change | |---|---|---| | [node](https://nodejs.org) ([source](https://github.com/nodejs/node)) | patch | `25.8.0` → `25.8.1` | MR created with the help of [el-capitano/tools/renovate-bot](https://gitlab.com/el-capitano/tools/renovate-bot). **Proposed changes to behavior should be submitted there as MRs.** --- ### Release Notes <details> <summary>nodejs/node (node)</summary> ### [`v25.8.1`](https://github.com/nodejs/node/releases/tag/v25.8.1): 2026-03-11, Version 25.8.1 (Current), @​aduh95 [Compare Source](nodejs/node@v25.8.0...v25.8.1) ##### Notable Changes - \[[`ea87eea71a`](nodejs/node@ea87eea71a)] - **module**: fix extensionless CJS files in `"type": "module"` packages (Matteo Collina) [#​62083](nodejs/node#62083) ##### Commits - \[[`bab750d1b3`](nodejs/node@bab750d1b3)] - **build**: do not depend on V8 deps on `--without-bundled-v8` builds (Antoine du Hamel) [#​62033](nodejs/node#62033) - \[[`b26d1c7fcb`](nodejs/node@b26d1c7fcb)] - **crypto**: make --use-system-ca per-env rather than per-process (Aditi) [#​60678](nodejs/node#60678) - \[[`e362635abf`](nodejs/node@e362635abf)] - **crypto**: add missing AES dictionaries (Filip Skokan) [#​62099](nodejs/node#62099) - \[[`6f975db8af`](nodejs/node@6f975db8af)] - **crypto**: fix importKey required argument count check (Filip Skokan) [#​62099](nodejs/node#62099) - \[[`3beaf9c5fc`](nodejs/node@3beaf9c5fc)] - **deps**: update amaro to 1.1.8 (Node.js GitHub Bot) [#​62151](nodejs/node#62151) - \[[`53afb0edd8`](nodejs/node@53afb0edd8)] - **deps**: update sqlite to 3.52.0 (Node.js GitHub Bot) [#​62150](nodejs/node#62150) - \[[`a13ed052a1`](nodejs/node@a13ed052a1)] - **deps**: update merve to 1.2.0 (Node.js GitHub Bot) [#​62149](nodejs/node#62149) - \[[`2c850577b7`](nodejs/node@2c850577b7)] - **deps**: patch resb crate (Richard Lau) [#​62138](nodejs/node#62138) - \[[`37862a6728`](nodejs/node@37862a6728)] - **deps**: V8: cherry-pick [`aa0b288`](nodejs/node@aa0b288f87cc) (Richard Lau) [#​62136](nodejs/node#62136) - \[[`09191ad8b4`](nodejs/node@09191ad8b4)] - **deps**: update ada to 3.4.3 (Node.js GitHub Bot) [#​62049](nodejs/node#62049) - \[[`8d63a178fd`](nodejs/node@8d63a178fd)] - **doc**: copyedit `addons.md` (Antoine du Hamel) [#​62071](nodejs/node#62071) - \[[`83719ffb64`](nodejs/node@83719ffb64)] - **doc**: correct `util.convertProcessSignalToExitCode` validation behavior (René) [#​62134](nodejs/node#62134) - \[[`eeee7c7fb1`](nodejs/node@eeee7c7fb1)] - **doc**: add efekrskl as triager (Efe) [#​61876](nodejs/node#61876) - \[[`db150b2e69`](nodejs/node@db150b2e69)] - **doc**: fix markdown for `expectFailure` values (Jacob Smith) [#​62100](nodejs/node#62100) - \[[`d55a441e60`](nodejs/node@d55a441e60)] - **doc**: add title to index (Aviv Keller) [#​62046](nodejs/node#62046) - \[[`cc46204b48`](nodejs/node@cc46204b48)] - **doc**: include url.resolve() in DEP0169 application deprecation (Mike McCready) [#​62002](nodejs/node#62002) - \[[`1d91a7261e`](nodejs/node@1d91a7261e)] - **doc,module**: add missing doc for syncHooks.deregister() (Joyee Cheung) [#​61959](nodejs/node#61959) - \[[`5198573bee`](nodejs/node@5198573bee)] - **http**: fix use-after-free when freeParser is called during llhttp\_execute (Gerhard Stöbich) [#​62095](nodejs/node#62095) - \[[`f8793f80df`](nodejs/node@f8793f80df)] - **lib**: fix source map url parse in dynamic imports (Chengzhong Wu) [#​61990](nodejs/node#61990) - \[[`5439d0e0cf`](nodejs/node@5439d0e0cf)] - **meta**: bump actions/download-artifact from 7.0.0 to 8.0.0 (dependabot\[bot]) [#​62063](nodejs/node#62063) - \[[`27fd21943a`](nodejs/node@27fd21943a)] - **meta**: bump actions/upload-artifact from 6.0.0 to 7.0.0 (dependabot\[bot]) [#​62062](nodejs/node#62062) - \[[`5b266f3295`](nodejs/node@5b266f3295)] - **meta**: bump step-security/harden-runner from 2.14.2 to 2.15.0 (dependabot\[bot]) [#​62064](nodejs/node#62064) - \[[`ea87eea71a`](nodejs/node@ea87eea71a)] - **module**: fix extensionless CJS files in `"type": "module"` packages (Matteo Collina) [#​62083](nodejs/node#62083) - \[[`851228cd60`](nodejs/node@851228cd60)] - **sqlite**: handle stmt invalidation (Guilherme Araújo) [#​61877](nodejs/node#61877) - \[[`19efe60548`](nodejs/node@19efe60548)] - **src**: expose async context frame debugging helper to JS (Anna Henningsen) [#​62103](nodejs/node#62103) - \[[`0257e8072f`](nodejs/node@0257e8072f)] - **src**: make AsyncWrap subclass internal field counts explicit (Anna Henningsen) [#​62103](nodejs/node#62103) - \[[`975dafbe3b`](nodejs/node@975dafbe3b)] - **src**: release context frame in AsyncWrap::EmitDestroy (Gerhard Stöbich) [#​61995](nodejs/node#61995) - \[[`f2c08c7888`](nodejs/node@f2c08c7888)] - **src**: use validate\_ascii\_with\_errors instead of validate\_ascii (Сковорода Никита Андреевич) [#​61122](nodejs/node#61122) - \[[`0278461d83`](nodejs/node@0278461d83)] - **stream**: optimize webstreams pipeTo (Mattias Buelens) [#​62079](nodejs/node#62079) - \[[`4d62e95bfa`](nodejs/node@4d62e95bfa)] - **stream**: fix brotli error handling in web compression streams (Filip Skokan) [#​62107](nodejs/node#62107) - \[[`4bdcaf2865`](nodejs/node@4bdcaf2865)] - **stream**: improve Web Compression spec compliance (Filip Skokan) [#​62107](nodejs/node#62107) - \[[`a5b1be2045`](nodejs/node@a5b1be2045)] - **stream**: fix UTF-8 character corruption in fast-utf8-stream (Matteo Collina) [#​61745](nodejs/node#61745) - \[[`5632446c4e`](nodejs/node@5632446c4e)] - **stream**: fix TransformStream race on cancel with pending write (Marco) [#​62040](nodejs/node#62040) - \[[`f90fa9cd1a`](nodejs/node@f90fa9cd1a)] - **stream**: accept ArrayBuffer in CompressionStream and DecompressionStream (조수민) [#​61913](nodejs/node#61913) - \[[`00319eaa3a`](nodejs/node@00319eaa3a)] - **test**: update WPT for url to [`c928b19`](nodejs/node@c928b19ab0) (Node.js GitHub Bot) [#​62148](nodejs/node#62148) - \[[`456abc7d20`](nodejs/node@456abc7d20)] - **test**: update WPT for WebCryptoAPI to [`c9e9558`](nodejs/node@c9e955840a) (Node.js GitHub Bot) [#​62147](nodejs/node#62147) - \[[`82770cb7d3`](nodejs/node@82770cb7d3)] - **test**: improve WPT report runner (Filip Skokan) [#​62107](nodejs/node#62107) - \[[`cfc847d233`](nodejs/node@cfc847d233)] - **test**: update WPT compression to [`ae05f5c`](nodejs/node@ae05f5cb53) (Filip Skokan) [#​62107](nodejs/node#62107) - \[[`80f78f2737`](nodejs/node@80f78f2737)] - **test**: update WPT for WebCryptoAPI to [`42e4732`](nodejs/node@42e47329fd) (Node.js GitHub Bot) [#​62048](nodejs/node#62048) - \[[`8048e0508c`](nodejs/node@8048e0508c)] - **test**: fix skipping behavior for `test-runner-run-files-undefined` (Antoine du Hamel) [#​62026](nodejs/node#62026) - \[[`699a6214c6`](nodejs/node@699a6214c6)] - **tools**: revert timezone update GHA workflow to ubuntu-latest (Richard Lau) [#​62140](nodejs/node#62140) - \[[`1a453b550c`](nodejs/node@1a453b550c)] - **tools**: improve error handling in test426 update script (Rich Trott) [#​62121](nodejs/node#62121) - \[[`710dde5ee2`](nodejs/node@710dde5ee2)] - **tools**: fix `--node-builtin-modules-path` value in `shell.nix` (Antoine du Hamel) [#​62102](nodejs/node#62102) - \[[`dcb1cbb21f`](nodejs/node@dcb1cbb21f)] - **tools**: bump the eslint group across 1 directory with 2 updates (dependabot\[bot]) [#​62092](nodejs/node#62092) - \[[`7d0b758583`](nodejs/node@7d0b758583)] - **tools**: fix daily wpt workflow nighly release version lookup (Filip Skokan) [#​62076](nodejs/node#62076) - \[[`3e8c816f2e`](nodejs/node@3e8c816f2e)] - **tools**: fix example in release proposal linter (Richard Lau) [#​62074](nodejs/node#62074) - \[[`772d3d270d`](nodejs/node@772d3d270d)] - **tools**: bump minimatch from 3.1.3 to 3.1.5 in /tools/clang-format (dependabot\[bot]) [#​62013](nodejs/node#62013) - \[[`92f3b42672`](nodejs/node@92f3b42672)] - **tools**: bump eslint to v10, babel to v8.0.0-rc.2 (Huáng Jùnliàng) [#​61905](nodejs/node#61905) - \[[`deead95ec5`](nodejs/node@deead95ec5)] - **url**: suppress warnings from url.format/url.resolve inside node\_modules (René) [#​62005](nodejs/node#62005) </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Enabled. ♻ **Rebasing**: Whenever MR is behind base branch, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this MR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this MR, check this box --- This MR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiI0My42MS43IiwidXBkYXRlZEluVmVyIjoiNDMuNjEuNyIsInRhcmdldEJyYW5jaCI6Im1haW4iLCJsYWJlbHMiOlsiUmVub3ZhdGUgQm90IiwiYXV0b21hdGlvbjpib3QtYXV0aG9yZWQiLCJkZXBlbmRlbmN5LXR5cGU6OnBhdGNoIl19-->
Release the async context frame in
AsyncWrap::EmitDestroyto allow gc to collect it.This is in special relevant for reused resources like
HTTPParserotherwise they might keep ALS stores alive.Fixes: #61882
Refs: #48528