Skip to content

stream: preserve asynclocalstorage context in finished() - #57865

Merged
nodejs-github-bot merged 4 commits into
nodejs:mainfrom
gurgunday:fix/async-storage-stream-finished
Apr 16, 2025
Merged

stream: preserve asynclocalstorage context in finished()#57865
nodejs-github-bot merged 4 commits into
nodejs:mainfrom
gurgunday:fix/async-storage-stream-finished

Conversation

@gurgunday

@gurgundaygurgunday commented Apr 13, 2025•
edited
Loading

Copy link
Copy Markdown
Member

Fixes#57844

@nodejs-github-bot

nodejs-github-bot commented Apr 13, 2025•
edited by rluvaton
Loading

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/streams

@nodejs-github-botnodejs-github-bot added needs-ci PRs that need a full CI run. stream Issues and PRs related to the stream subsystem. labels Apr 13, 2025
@jasnell

jasnell commented Apr 13, 2025

Copy link
Copy Markdown
Member

Inside the finished(...) function definition, you ought to be able to simply use AsyncLocalStore.bind(...) to wrap the callback function such that the current context is preserved. Essentially equivalent to the following except the AsyncLocalStorage.bind(...) call would be inside the finished(...) function body.

const{ finished, Readable }=require('stream');const{ AsyncLocalStorage }=require('async_hooks');constals=newAsyncLocalStorage();constreadable=newReadable();als.run(123,()=>{finished(readable,AsyncLocalStorage.bind(()=>console.log(als.getStore())));});readable.destroy();

@gurgunday
gurgundayforce-pushed the fix/async-storage-stream-finished branch from 783785d to a29d44dCompareApril 13, 2025 18:16
@gurgunday

gurgunday commented Apr 13, 2025

Copy link
Copy Markdown
MemberAuthor

@jasnell oh you're absolutely right, I missed that, thanks!

jasnell
jasnell reviewed Apr 13, 2025
View reviewed changes
Comment threadlib/internal/streams/end-of-stream.js Outdated
jasnell
jasnell reviewed Apr 13, 2025
View reviewed changes
Comment threadtest/async-hooks/test-async-local-storage-stream-finished.js Outdated
@codecov

codecovBot commented Apr 13, 2025•
edited
Loading

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.25%. Comparing base (964e41c) to head (cd26628).
Report is 444 commits behind head on main.

Additional details and impacted files
@@ Coverage Diff @@## main #57865 +/- ##
==========================================
+ Coverage 90.23% 90.25% +0.02% 
==========================================
Files 630 630 Lines 185518 185674 +156 Branches 36369 36401 +32 ==========================================
+ Hits 167401 167580 +179 + Misses 11005 10992 -13 + Partials 7112 7102 -10
Files with missing linesCoverage Δ
lib/internal/streams/end-of-stream.js97.28% <100.00%> (+0.03%)⬆️

... and 27 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

bjohansebas
bjohansebas approved these changes Apr 13, 2025
View reviewed changes

@bjohansebasbjohansebas left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks good, thanks for working on this

Also, lgtm if the CI passes

@bjohansebasbjohansebas added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 13, 2025
@github-actionsgithub-actionsBot added request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. and removed request-ci Add this label to start a Jenkins CI on a PR. labels Apr 13, 2025
@github-actions

github-actionsBot commented Apr 13, 2025

Copy link
Copy Markdown
Contributor
Failed to start CI
 ⚠ No approving reviews found
✘ Refusing to run CI on potentially unsafe PR
https://github.com/nodejs/node/actions/runs/14433881994

rluvaton
rluvaton approved these changes Apr 13, 2025
View reviewed changes

@rluvatonrluvaton left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Comment threadlib/internal/streams/end-of-stream.js Outdated
@rluvatonrluvaton added request-ci Add this label to start a Jenkins CI on a PR. and removed request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. labels Apr 13, 2025
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 13, 2025
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
mcollina
mcollina approved these changes Apr 14, 2025
View reviewed changes

@mcollinamcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@mcollinamcollina added semver-minor PRs that contain new features and should be released in the next minor version. request-ci Add this label to start a Jenkins CI on a PR. labels Apr 14, 2025
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 14, 2025
@nodejs-github-bot

nodejs-github-bot commented Apr 14, 2025•
edited by jasnell
Loading

Copy link
Copy Markdown
Collaborator

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

@mcollinamcollina added the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 14, 2025
@nodejs-github-botnodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 16, 2025
@nodejs-github-bot
nodejs-github-bot merged commit 4b2b3c0 into nodejs:mainApr 16, 2025
@nodejs-github-bot

nodejs-github-bot commented Apr 16, 2025

Copy link
Copy Markdown
Collaborator

Landed in 4b2b3c0

RafaelGSS pushed a commit that referenced this pull request May 1, 2025
@gurgunday@RafaelGSS
stream: preserve AsyncLocalStorage context in finished()
56336ee
PR-URL: #57865
Reviewed-By: Raz Luvaton <rluvaton@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
@RafaelGSSRafaelGSS mentioned this pull request May 1, 2025
Merged
RafaelGSS pushed a commit that referenced this pull request May 2, 2025
@gurgunday@RafaelGSS
stream: preserve AsyncLocalStorage context in finished()
932c2d9
PR-URL: #57865
Reviewed-By: Raz Luvaton <rluvaton@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
aduh95 pushed a commit that referenced this pull request May 6, 2025
@gurgunday@aduh95
stream: preserve AsyncLocalStorage context in finished()
e24e097
PR-URL: #57865
Reviewed-By: Raz Luvaton <rluvaton@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
@slagiewkaslagiewka mentioned this pull request May 12, 2025
Closed
RafaelGSS pushed a commit that referenced this pull request May 14, 2025
@gurgunday@RafaelGSS
stream: preserve AsyncLocalStorage context in finished()
3720af2
PR-URL: #57865
Reviewed-By: Raz Luvaton <rluvaton@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
@bjohansebasbjohansebas added lts-watch-v20.x lts-watch-v22.x PRs that may need to be released in v22.x labels May 15, 2025
aduh95 pushed a commit that referenced this pull request May 16, 2025
@gurgunday@aduh95
stream: preserve AsyncLocalStorage context in finished()
319cdeb
PR-URL: #57865
Reviewed-By: Raz Luvaton <rluvaton@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
aduh95 pushed a commit that referenced this pull request May 17, 2025
@gurgunday@aduh95
stream: preserve AsyncLocalStorage context in finished()
6d7b734
PR-URL: #57865
Reviewed-By: Raz Luvaton <rluvaton@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
aduh95 pushed a commit that referenced this pull request May 18, 2025
@gurgunday@aduh95
stream: preserve AsyncLocalStorage context in finished()
3fb2f7b
PR-URL: #57865
Reviewed-By: Raz Luvaton <rluvaton@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
@aduh95aduh95 removed the lts-watch-v22.x PRs that may need to be released in v22.x label May 19, 2025
@github-actionsgithub-actionsBot mentioned this pull request May 19, 2025
Merged
aduh95 pushed a commit that referenced this pull request May 19, 2025
@gurgunday@aduh95
stream: preserve AsyncLocalStorage context in finished()
0ec912f
PR-URL: #57865
Reviewed-By: Raz Luvaton <rluvaton@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
aduh95 added a commit that referenced this pull request May 19, 2025
@aduh95
2025-05-21, Version 22.16.0 'Jod' (LTS)
9774069
Notable changes:
deps:
* update timezone to 2025b (Node.js GitHub Bot) #57857
doc:
* add dario-piotrowicz to collaborators (Dario Piotrowicz) #58102
* (SEMVER-MINOR) graduate multiple experimental apis (James M Snell) #57765
esm:
* (SEMVER-MINOR) graduate import.meta properties (James M Snell) #58011
* (SEMVER-MINOR) support top-level Wasm without package type (Guy Bedford) #57610
sqlite:
* (SEMVER-MINOR) add StatementSync.prototype.columns() (Colin Ihrig) #57490
src:
* (SEMVER-MINOR) set default config as `node.config.json` (Marco Ippolito) #57171
* (SEMVER-MINOR) create `THROW_ERR_OPTIONS_BEFORE_BOOTSTRAPPING` (Marco Ippolito) #57016
* (SEMVER-MINOR) add config file support (Marco Ippolito) #57016
* (SEMVER-MINOR) add ExecutionAsyncId getter for any Context (Attila Szegedi) #57820
stream:
* (SEMVER-MINOR) preserve AsyncLocalStorage context in finished() (Gürgün Dayıoğlu) #57865
util:
* (SEMVER-MINOR) add `types.isFloat16Array()` (Livia Medeiros) #57879
worker:
* (SEMVER-MINOR) add worker.getHeapStatistics() (Matteo Collina) #57888
PR-URL: #58388
aduh95 added a commit that referenced this pull request May 21, 2025
@aduh95
2025-05-21, Version 22.16.0 'Jod' (LTS)
0bd9680
Notable changes:
deps:
* update timezone to 2025b (Node.js GitHub Bot) #57857
doc:
* add dario-piotrowicz to collaborators (Dario Piotrowicz) #58102
* (SEMVER-MINOR) graduate multiple experimental apis (James M Snell) #57765
esm:
* (SEMVER-MINOR) graduate import.meta properties (James M Snell) #58011
* (SEMVER-MINOR) support top-level Wasm without package type (Guy Bedford) #57610
sqlite:
* (SEMVER-MINOR) add StatementSync.prototype.columns() (Colin Ihrig) #57490
src:
* (SEMVER-MINOR) set default config as `node.config.json` (Marco Ippolito) #57171
* (SEMVER-MINOR) create `THROW_ERR_OPTIONS_BEFORE_BOOTSTRAPPING` (Marco Ippolito) #57016
* (SEMVER-MINOR) add config file support (Marco Ippolito) #57016
* (SEMVER-MINOR) add ExecutionAsyncId getter for any Context (Attila Szegedi) #57820
stream:
* (SEMVER-MINOR) preserve AsyncLocalStorage context in finished() (Gürgün Dayıoğlu) #57865
util:
* (SEMVER-MINOR) add `types.isFloat16Array()` (Livia Medeiros) #57879
worker:
* (SEMVER-MINOR) add worker.getHeapStatistics() (Matteo Collina) #57888
PR-URL: #58388
marco-ippolito pushed a commit that referenced this pull request Jun 5, 2025
@gurgunday@marco-ippolito
stream: preserve AsyncLocalStorage context in finished()
89f9729
PR-URL: #57865
Reviewed-By: Raz Luvaton <rluvaton@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
@ghostghost mentioned this pull request Jun 8, 2025
Closed
@AlCalzoneAlCalzone mentioned this pull request May 13, 2026
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcollinamcollinamcollina approved these changes
@jasnelljasnelljasnell approved these changes
@legendecaslegendecaslegendecas approved these changes
@BridgeARBridgeARBridgeAR approved these changes
@rluvatonrluvatonrluvaton approved these changes
+1 more reviewer
@bjohansebasbjohansebasbjohansebas approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

async_local_storageAsyncLocalStorageauthor readyPRs that have at least one approval, no pending requests for changes, and a CI started.commit-queue-squashAdd this label to instruct the Commit Queue to squash all the PR commits into the first one.needs-ciPRs that need a full CI run.semver-minorPRs that contain new features and should be released in the next minor version.streamIssues and PRs related to the stream subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

cannot read stored value in asynclocalstorage when using finished

10 participants

@gurgunday@nodejs-github-bot@jasnell@bjohansebas@mcollina@legendecas@BridgeAR@rluvaton@aduh95@marco-ippolito