Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

worker: flush stdout and stderr on exit #56428

Merged
merged 4 commits into from
Jan 6, 2025

Conversation

mcollina
Copy link
Member

@mcollina mcollina commented Jan 1, 2025

This fix an old problem I had since forever when using worker threads, stdout log lines being cut if the thread exited. This PR fixes it by bypassing backpressure if the thread is exiting.

Signed-off-by: Matteo Collina <[email protected]>
@mcollina mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 1, 2025
@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. worker Issues and PRs related to Worker support. labels Jan 1, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 1, 2025
@nodejs-github-bot

This comment was marked as outdated.

Copy link

codecov bot commented Jan 2, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.53%. Comparing base (ca69d0a) to head (de200d8).
Report is 64 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #56428      +/-   ##
==========================================
- Coverage   88.54%   88.53%   -0.01%     
==========================================
  Files         657      658       +1     
  Lines      190393   190822     +429     
  Branches    36552    36627      +75     
==========================================
+ Hits       168582   168947     +365     
- Misses      14998    15048      +50     
- Partials     6813     6827      +14     
Files with missing lines Coverage Δ
.../internal/bootstrap/switches/is_not_main_thread.js 100.00% <100.00%> (ø)
lib/internal/worker/io.js 99.17% <100.00%> (+<0.01%) ⬆️

... and 70 files with indirect coverage changes

Copy link
Contributor

@ShogunPanda ShogunPanda left a comment

Choose a reason for hiding this comment

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

Other than linting, LGTM.

Signed-off-by: Matteo Collina <[email protected]>
Signed-off-by: Matteo Collina <[email protected]>
@mcollina mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 5, 2025
@mcollina
Copy link
Member Author

mcollina commented Jan 5, 2025

@jasnell @ShogunPanda PTAL, this should be good now.

@mcollina
Copy link
Member Author

mcollina commented Jan 5, 2025

I've handled another bad case I identified.

@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 5, 2025
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot
Copy link
Collaborator

@mcollina mcollina added commit-queue Add this label to land a pull request using GitHub Actions. lts-watch-v20.x PRs that may need to be released in v20.x lts-watch-v22.x PRs that may need to be released in v22.x and removed needs-ci PRs that need a full CI run. labels Jan 5, 2025
@nodejs-github-bot nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Jan 5, 2025
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/56428
✔  Done loading data for nodejs/node/pull/56428
----------------------------------- PR info ------------------------------------
Title      worker: flush stdout and stderr on exit (#56428)
Author     Matteo Collina <[email protected]> (@mcollina)
Branch     mcollina:worker-thread-stdout -> nodejs:main
Labels     worker, lts-watch-v20.x, lts-watch-v22.x
Commits    4
 - worker: flush stdout and stderr on exit
 - fixup
 - fixup
 - fixup
Committers 1
 - Matteo Collina <[email protected]>
PR-URL: https://github.com/nodejs/node/pull/56428
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Paolo Insogna <[email protected]>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/56428
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Paolo Insogna <[email protected]>
--------------------------------------------------------------------------------
   ℹ  This PR was created on Wed, 01 Jan 2025 22:54:12 GMT
   ✔  Approvals: 2
   ✔  - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/56428#pullrequestreview-2530942296
   ✔  - Paolo Insogna (@ShogunPanda) (TSC): https://github.com/nodejs/node/pull/56428#pullrequestreview-2527130910
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2025-01-05T18:01:05Z: https://ci.nodejs.org/job/node-test-pull-request/64354/
- Querying data for job/node-test-pull-request/64354/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  No git cherry-pick in progress
   ✔  No git am in progress
   ✔  No git rebase in progress
--------------------------------------------------------------------------------
- Bringing origin/main up to date...
From https://github.com/nodejs/node
 * branch                  main       -> FETCH_HEAD
✔  origin/main is now up-to-date
- Downloading patch for 56428
From https://github.com/nodejs/node
 * branch                  refs/pull/56428/merge -> FETCH_HEAD
✔  Fetched commits as b736028c7f62..de200d845ba6
--------------------------------------------------------------------------------
[main 42373cf1d9] worker: flush stdout and stderr on exit
 Author: Matteo Collina <[email protected]>
 Date: Thu Dec 26 13:42:47 2024 +0100
 1 file changed, 28 insertions(+)
 create mode 100644 test/parallel/test-worker-stdio-flush.js
[main 1e1e3e5ca0] fixup
 Author: Matteo Collina <[email protected]>
 Date: Wed Jan 1 17:52:26 2025 -0500
 2 files changed, 4 insertions(+), 1 deletion(-)
[main 32a1594f30] fixup
 Author: Matteo Collina <[email protected]>
 Date: Sat Jan 4 20:23:48 2025 -0500
 3 files changed, 43 insertions(+), 4 deletions(-)
 create mode 100644 test/parallel/test-worker-stdio-flush-inflight.js
[main 43ecc894e7] fixup
 Author: Matteo Collina <[email protected]>
 Date: Sat Jan 4 21:01:08 2025 -0500
 3 files changed, 21 insertions(+), 27 deletions(-)
   ✔  Patches applied
There are 4 commits in the PR. Attempting autorebase.
Rebasing (2/8)
Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
worker: flush stdout and stderr on exit

Signed-off-by: Matteo Collina <[email protected]>
PR-URL: #56428
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Paolo Insogna <[email protected]>

[detached HEAD 7ad6e696fe] worker: flush stdout and stderr on exit
Author: Matteo Collina <[email protected]>
Date: Thu Dec 26 13:42:47 2024 +0100
1 file changed, 28 insertions(+)
create mode 100644 test/parallel/test-worker-stdio-flush.js
Rebasing (3/8)
Rebasing (4/8)
Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
fixup

Signed-off-by: Matteo Collina <[email protected]>
PR-URL: #56428
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Paolo Insogna <[email protected]>

[detached HEAD af5f7a08e6] fixup
Author: Matteo Collina <[email protected]>
Date: Wed Jan 1 17:52:26 2025 -0500
2 files changed, 4 insertions(+), 1 deletion(-)
Rebasing (5/8)
Rebasing (6/8)
Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
fixup

Signed-off-by: Matteo Collina <[email protected]>
PR-URL: #56428
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Paolo Insogna <[email protected]>

[detached HEAD 2a4349e949] fixup
Author: Matteo Collina <[email protected]>
Date: Sat Jan 4 20:23:48 2025 -0500
3 files changed, 43 insertions(+), 4 deletions(-)
create mode 100644 test/parallel/test-worker-stdio-flush-inflight.js
Rebasing (7/8)
Rebasing (8/8)
Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
fixup

Signed-off-by: Matteo Collina <[email protected]>
PR-URL: #56428
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Paolo Insogna <[email protected]>

[detached HEAD 83fe3c2301] fixup
Author: Matteo Collina <[email protected]>
Date: Sat Jan 4 21:01:08 2025 -0500
3 files changed, 21 insertions(+), 27 deletions(-)
Successfully rebased and updated refs/heads/main.

ℹ Add commit-queue-squash label to land the PR as one commit, or commit-queue-rebase to land as separate commits.

https://github.com/nodejs/node/actions/runs/12623476751

@lpinca lpinca added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Jan 6, 2025
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 6, 2025
@nodejs-github-bot nodejs-github-bot merged commit b0c65bb into nodejs:main Jan 6, 2025
81 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in b0c65bb

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. lts-watch-v20.x PRs that may need to be released in v20.x lts-watch-v22.x PRs that may need to be released in v22.x worker Issues and PRs related to Worker support.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants