-
Notifications
You must be signed in to change notification settings - Fork 13.6k
std: thread: Return error if setting thread stack size fails #144210
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: master
Are you sure you want to change the base?
Conversation
r? @ibraheemdev rustbot has assigned @ibraheemdev. Use |
CI failure is unrelated and spurious, could you perhaps retry it? |
8e5114a
to
e143986
Compare
Rebased to squash the fixup and re-trigger CI. |
Two weeks passed, I guess I'll re-roll reviewers. r? libs |
@bors r+ Tagging as release notes as this is something that may be worth calling out. |
…r=jhpratt std: thread: Return error if setting thread stack size fails Currently, when setting the thread stack size fails, it would be rounded up to the nearest multiple of the page size and the code asserts that the next call to `pthread_attr_setstacksize` succeeds. This may be true for glibc, but it isn't true for musl, which not only enforces a minimum stack size, but also a maximum stack size of `usize::MAX / 4 - PTHREAD_STACK_MIN` [1], triggering the assert rather than erroring gracefully. There isn't any way to handle this properly other than bailing out and letting the user know it didn't succeed. [1]: https://git.musl-libc.org/cgit/musl/tree/src/thread/pthread_attr_setstacksize.c#n5
…r=jhpratt std: thread: Return error if setting thread stack size fails Currently, when setting the thread stack size fails, it would be rounded up to the nearest multiple of the page size and the code asserts that the next call to `pthread_attr_setstacksize` succeeds. This may be true for glibc, but it isn't true for musl, which not only enforces a minimum stack size, but also a maximum stack size of `usize::MAX / 4 - PTHREAD_STACK_MIN` [1], triggering the assert rather than erroring gracefully. There isn't any way to handle this properly other than bailing out and letting the user know it didn't succeed. [1]: https://git.musl-libc.org/cgit/musl/tree/src/thread/pthread_attr_setstacksize.c#n5
This comment has been minimized.
This comment has been minimized.
💔 Test for 63955d1 failed: CI. Failed jobs:
|
This comment has been minimized.
This comment has been minimized.
Failed in rollup: #145285 (comment) @bors r- |
See also #145299, which aims to make the relevant test insensitive to thread scheduling. |
I don't really see how that fail would be related at all, this PR only changes an assert to an error. The test doesn't rely on this assert, so I assume it was already broken before? |
I thought it might have been flaky too, but it failed twice in rollup, and then failed again with just this PR. 🤷♂️ I agree that the test is probably broken. Maybe this change is unlucky enough to perturb codegen in a way that happens to trigger the pre-existing problem. |
Might as well do another try job to see if we can catch the test being flaky instead of just failing: @bors try jobs=x86_64-gnu-aux |
This comment has been minimized.
This comment has been minimized.
std: thread: Return error if setting thread stack size fails try-job: x86_64-gnu-aux
This comment has been minimized.
This comment has been minimized.
💔 Test for 5266eb8 failed: CI. Failed jobs:
|
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.
Wouldn't ErrorKind::InvalidInput
be more correct?
The failing test has been updated by #145299. |
Currently, when setting the thread stack size fails, it would be rounded up to the nearest multiple of the page size and the code asserts that the next call to pthread_attr_setstacksize succeeds. This may be true for glibc, but it isn't true for musl, which not only enforces a minimum stack size, but also a maximum stack size of usize::MAX / 4 - PTHREAD_STACK_MIN [1], triggering the assert rather than erroring gracefully. There isn't any way to handle this properly other than bailing out and letting the user know it didn't succeed. [1]: https://git.musl-libc.org/cgit/musl/tree/src/thread/pthread_attr_setstacksize.c#n5 Signed-off-by: Jens Reidel <[email protected]>
25fe461
to
5d01d90
Compare
Rebased to include the fix for the failing test and to address the feedback from @joboet |
oops, forgot to: @rustbot ready |
@bors try jobs=x86_64-gnu-aux |
This comment has been minimized.
This comment has been minimized.
std: thread: Return error if setting thread stack size fails try-job: x86_64-gnu-aux
@bors r+ |
Currently, when setting the thread stack size fails, it would be rounded up to the nearest multiple of the page size and the code asserts that the next call to
pthread_attr_setstacksize
succeeds.This may be true for glibc, but it isn't true for musl, which not only enforces a minimum stack size, but also a maximum stack size of
usize::MAX / 4 - PTHREAD_STACK_MIN
1, triggering the assert rather than erroring gracefully.There isn't any way to handle this properly other than bailing out and letting the user know it didn't succeed.