Skip to content

src: remove overzealous tcsetattr error check #58200

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

Merged
merged 1 commit into from
May 8, 2025

Conversation

bnoordhuis
Copy link
Member

Node calls tcsetattr on exit to reset the tty to its state on program start. Good idea in general but tcsetattr can fail for a number of reasons and since there really isn't anything we can do about it at that point, simply ignore the error instead of aborting with an inscrutable error message.

Most of the time it'll be fine because the most common failure is when the user has already logged off and there isn't anything to restore in the first place.

Fixes: #51519

Node calls tcsetattr on exit to reset the tty to its state on program
start. Good idea in general but tcsetattr can fail for a number of
reasons and since there really isn't anything we can do about it
at that point, simply ignore the error instead of aborting with
an inscrutable error message.

Most of the time it'll be fine because the most common failure is when
the user has already logged off and there isn't anything to restore in
the first place.

Fixes: nodejs#51519
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/startup

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels May 6, 2025
Copy link

codecov bot commented May 6, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.15%. Comparing base (4bfcad1) to head (ea99151).
Report is 20 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #58200   +/-   ##
=======================================
  Coverage   90.14%   90.15%           
=======================================
  Files         630      630           
  Lines      186784   186783    -1     
  Branches    36653    36659    +6     
=======================================
+ Hits       168377   168394   +17     
+ Misses      11205    11196    -9     
+ Partials     7202     7193    -9     
Files with missing lines Coverage Δ
src/node.cc 73.45% <ø> (+0.10%) ⬆️

... and 37 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.

@daeyeon daeyeon added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels May 6, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 6, 2025
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@bnoordhuis bnoordhuis added commit-queue Add this label to land a pull request using GitHub Actions. and removed needs-ci PRs that need a full CI run. labels May 8, 2025
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label May 8, 2025
@nodejs-github-bot nodejs-github-bot merged commit e532c86 into nodejs:main May 8, 2025
73 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in e532c86

targos pushed a commit that referenced this pull request May 16, 2025
Node calls tcsetattr on exit to reset the tty to its state on program
start. Good idea in general but tcsetattr can fail for a number of
reasons and since there really isn't anything we can do about it
at that point, simply ignore the error instead of aborting with
an inscrutable error message.

Most of the time it'll be fine because the most common failure is when
the user has already logged off and there isn't anything to restore in
the first place.

Fixes: #51519
PR-URL: #58200
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Daeyeon Jeong <daeyeon.dev@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ethan Arrowood <ethan@arrowood.dev>
aduh95 pushed a commit that referenced this pull request Jun 10, 2025
Node calls tcsetattr on exit to reset the tty to its state on program
start. Good idea in general but tcsetattr can fail for a number of
reasons and since there really isn't anything we can do about it
at that point, simply ignore the error instead of aborting with
an inscrutable error message.

Most of the time it'll be fine because the most common failure is when
the user has already logged off and there isn't anything to restore in
the first place.

Fixes: #51519
PR-URL: #58200
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Daeyeon Jeong <daeyeon.dev@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ethan Arrowood <ethan@arrowood.dev>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Node.js[5483]: ../src/node.cc:701:void node::ResetStdio(): Assertion `!(err != 0) || (err == -1 && (*__errno_location ()) == 1)' failed.
7 participants