-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
timeout: add all signal handlers, pass on signals to child, add ignored signal handling #10254
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
Conversation
93ea128 to
228824e
Compare
|
GNU testsuite comparison: |
|
GNU testsuite comparison: |
|
Another note is that we have a bunch of over-rides in the build-gnu script that override the yes and timeout command that we can remove once this is removed. |
CodSpeed Performance ReportMerging this PR will not alter performanceComparing Summary
Footnotes
|
|
@Ecordonnier Thanks for the feedback! This PR is a bit too complicated, I was thinking that the best approach here would be to add that pipe signal logic in a smaller PR to every utility. That would resolve 5+ pr's in the queue, then rebasing this one after addressing your feedback and then it would be better to understand |
you can do it that way if it makes sense short-term, but then you will have the same code duplicated 100 times (one time for each utility), which is not great for long-term maintenance. |
|
Whoops, what I meant to say is to add it to every utility by adding it once in the bin macro to not have to have the code duplicated. |
|
I'm rebasing it now after the changes to add the pipeline signal handlers to all of the utilities, should be a much smaller diff now |
fa027b0 to
293ded3
Compare
|
GNU testsuite comparison: |
|
I think the flakiness with the tee test should be resolved with this PR now. The latest change to make the yes utility have the PIPE signal handler, caused the test to fail because of this line: Now we can remove that line, and it makes the tee test pass and I believe now with the timeout changes, it removes the source of the flakness |
293ded3 to
4421852
Compare
|
GNU testsuite comparison: |
|
Do you still want #10166 to be merged? It overlaps with this PR. |
|
This one covers everything, I can close that one |
| match exec(&buffer) { | ||
| Ok(()) => Ok(()), | ||
| // On Windows, silently handle broken pipe since there's no SIGPIPE | ||
| #[cfg(windows)] |
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.
let's move this to another PR, as it's unrelated to the rest. and please also remove this from the PR description "I also had to enable to pipe signal handlers to the yes utility ".
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.
The timeout gnu tests validate that when timeout yes is run with sigpipe ignored that it exits with broken pipe, I kept it because that line needs to be removed for unix to pass the gnu integ tests but it's required for windows to pass the rust integ tests we've made for it.
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.
Added this in another PR here: #10429
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.
Which rust integ test do you mean?
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.
Answering my own question: test_yes.rs calls check_termination(), this calls "run_stdout_starts_with" in util.rs, and this calls "child.close_stdout()" and triggers a BrokenPipe error on windows.
Hence several tests are triggering BrokenPipe error on windows: test_yes::test_args, test_yes::test_long_input,
test_yes::test_long_odd_output, test_yes::test_long_output, test_yes::test_simple
|
GNU testsuite comparison: |
|
Removed the yes.rs change since it will be handled in the other PR, the timeout-group passes but the timeout gnu test fails since it relies on that yes change. Its split into the other PR |
|
@ChrisDryden I had restarted the one job which had failed 1 minute ago, but the merge of main cancelled the job and restarted all CI jobs ;-) |
|
GNU testsuite comparison: |
|
So timeout-group passed in the GNU test but isn't showing in the comments? |
At least in this PR the logs of the job "Run GNU tests (native)" clearly says "PASS: tests/timeout/timeout-group.sh" in the step "Run GNU Tests". But you are right something seems buggy in the comparison script. The step "Aggregate GNU test results" -> "compare test failures vs reference" says this in your PR: So somehow this is detected as a new test, and not as a test which was previously skipped. |
This is also what coreutils-tracking sees btw ( https://github.com/uutils/coreutils-tracking/blob/accf571e14c51cdcbd7f1a57521498506d061ed6/gnu-result.json#L20822C1-L20839C4 ): |
There was quite a bunch of different features missing in timeout, to start off, now that we have a mechanism to read the SIGPIPE handlers before they are overwritten by the rust runtime, it means that we can not propagate this signal down to the child processes if the signal is set to ignore.
This also includes all of the latest changes since 9.9 where the specific signal sent to timeout will be propagated instead of just defaulting to a TERM signal.