-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
timeout: Replace busy loop with select(2) #9227
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
base: main
Are you sure you want to change the base?
Conversation
1d7e3d0 to
b0b75e9
Compare
|
GNU testsuite comparison: |
CodSpeed Performance ReportMerging #9227 will not alter performanceComparing Summary
Footnotes
|
|
GNU testsuite comparison: |
|
I think I have most likely addressed the last macOS non-POSIX conformance bug, at last. Elsewhere, I will just ignore the only test that spuriously fails exclusively on obsolete macOS x86_64, and address it in the future; it is just a spurious failure on retrieving a child's exit code, because of a POSIX noncompliance bug on outdated macOS. This is most likely only an issue on how Rust's std handles the exit code retrieval, and not on real-world scenarios, so this is absolutely a non-issue compared to what this PR is fixing, sigh. I'll fix the build issue on Redox after macOS CI decides it's done bugging me and other maintainers. |
78e2c3b to
59e7efe
Compare
|
@cakebaker Do you have any suggestion/objection? I would like to get this issue fixed right away. I'll add some comments and rebase to latest main, then re-check CI because it was modified on main. Afterwards, unless CI says otherwise, this can be cleanly merged and close #9099. |
c245dbd to
a39b6bb
Compare
|
GNU testsuite comparison: |
|
GNU testsuite comparison: |
0a95bca to
9159c2d
Compare
|
GNU testsuite comparison: |
f717715 to
d062a3d
Compare
Fixed an issue where timeout would not force-kill due to wrong error handling on signal(), and also made error handle more robust for internal errors.
bf2c50d to
9643c59
Compare
|
GNU testsuite comparison: |
Now tests time out, so we can track regressions in timeout. Also added one to track nested timeouts. Co-authored-by: naoNao89 <[email protected]>
|
GNU testsuite comparison: |
|
GNU testsuite comparison: |
f0ae6a5 to
7504645
Compare
|
I don't know of a way that you can read the pipe signals to be able to handle the ignore signals and flags without it being overriden by the rust runtime without reading them in the .data_init section. Theres an implementation of it here: #9657 I think it would be much simpler to take this approach than whats in this PR. |
Oh, I didn't know about this. The thing is, timeout needs to scan all the signal space and block ignored ones to conform to GNU (something this PR is introducing), so I guess the best option is to just code in a special case for SIGPIPE that uses that approach. However, this PR has become quite expansive because it's fixing multiple bugs and adapting to new GNU behavior, so I guess it sticks. |
Also added a test case for it.
47d9aaa to
4004771
Compare
|
GNU testsuite comparison: |
|
GNU testsuite comparison: |
|
GNU testsuite comparison: |
|
GNU testsuite comparison: |
|
I swear macOS CI is so (demoralizingly) petty it has internal failures in purpose when it knows it's gonna pass lmao. |
Fixes issue #9099, where uu_timeout would have a static 100ms latency. This issue implements a self-pipe trick in order to run transparently on as many platforms as possible. There was a previous attempt of mine doing so with
pselect(), but this failed due to macOS shenanigans and the lack of support from Rust's std for spawning with custom signal masks (which was ACP-accepted two or three years ago, but even patching it I couldn't get it to work properly…).