-
Notifications
You must be signed in to change notification settings - Fork 29.7k
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
build: block SIGTTOU before calling tcsetattr() #28535
Conversation
Would it make sense to add a regression test for this as well? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason not to have a test? (I'm thinking there's no reason not to have something that checks if we're on Windows and skips, otherwise uses child_process
to run cat <(node -v)
. But maybe tty vs. no tty or whatever will mess stuff up? In that case, maybe accept error codes on exit, and the test just makes sure the command actually returns, error or no?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Why is the commit subsystem build
instead of src
?
We might be a background job that doesn't own the TTY so block SIGTTOU before making the tcsetattr() call, otherwise that signal suspends us. This is a better fix than PR nodejs#28490 for issue nodejs#28479. Fixes: nodejs#28530 Fixes: nodejs#28479 Refs: nodejs#28490
Yes. Two issues make writing a test hard:
IOW, I can't see a way to recreate the circumstances of the hang. If anyone has a good suggestion I'd like to hear it.
I have no idea, I have no recollection of writing that. :-) I've fixed it up. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. node doesn't have PTY and job control APIs, so I'm not surprised that its not possible to emulate what a job-control shell does to trigger this problem in our tests.
Test can check for existence of
Yeah, this one is harder unless running a bash script in the fixtures dir can somehow solve it? Anyway, totally not blocking on this. |
Just to confirm: I spent considerable time today trying to make it work and I didn't find a way. |
This comment has been minimized.
This comment has been minimized.
Landed in b06ce0b |
We might be a background job that doesn't own the TTY so block SIGTTOU before making the tcsetattr() call, otherwise that signal suspends us. This is a better fix than PR nodejs#28490 for issue nodejs#28479. Fixes: nodejs#28530 Fixes: nodejs#28479 Refs: nodejs#28490 PR-URL: nodejs#28535 Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
We might be a background job that doesn't own the TTY so block SIGTTOU before making the tcsetattr() call, otherwise that signal suspends us. This is a better fix than PR #28490 for issue #28479. Fixes: #28530 Fixes: #28479 Refs: #28490 PR-URL: #28535 Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
I think this was the wrong solution. The real bug here, IMO, is Node trying to call |
We might be a background job that doesn't own the TTY so block SIGTTOU
before making the tcsetattr() call, otherwise that signal suspends us.
This is a better fix than PR #28490 for issue #28479.
Fixes: #28530
Fixes: #28479
Refs: #28490