Skip to content
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

panic when encountering an illegal cpumask in thread::available_parallelism #115946

Merged
merged 1 commit into from
Sep 19, 2023

Conversation

the8472
Copy link
Member

@the8472 the8472 commented Sep 18, 2023

Fixes #115868 by panicking instead of returning an invalid NonZeroUsize

@the8472 the8472 added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Sep 18, 2023
@rustbot
Copy link
Collaborator

rustbot commented Sep 18, 2023

r? @Mark-Simulacrum

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added O-unix Operating system: Unix-like S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 18, 2023
@Mark-Simulacrum
Copy link
Member

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Sep 19, 2023

📌 Commit a6d8724 has been approved by Mark-Simulacrum

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 19, 2023
@WiSaGaN
Copy link
Contributor

WiSaGaN commented Sep 19, 2023

This function can error, and one of the error is

If the amount of parallelism is not known for the target platform.

Is it better to return an error of this kind and let user to decide whether to panic or not, instead of always panicing?

@the8472
Copy link
Member Author

the8472 commented Sep 19, 2023

This only happens when the system API violates its own API contract. That is different from the system API not being available or returning an explicit error.

And as commented on the code and in the associated issue this is only known to happen on an outdated kernel below our minimum supported version.... which means we don't need to support that.

@WiSaGaN
Copy link
Contributor

WiSaGaN commented Sep 19, 2023

I understand where you are coming from. I do however feel a bit of unease when something that is essentially a "get" function panics. It would be pretty trivial to recover from such error from a user point of view, e.g. just unwrap_or(1) if we return error here. If we panic here it would be much harder to recover, (catch panic?) from such a non-catastraphic error.

@the8472
Copy link
Member Author

the8472 commented Sep 19, 2023

Since the method already returns a Result people presumably already are using unwrap_or(1) which means they'd never learn about their kernel being broken.

bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 19, 2023
…llaumeGomez

Rollup of 6 pull requests

Successful merges:

 - rust-lang#112725 (rustdoc-search: add support for type parameters)
 - rust-lang#114941 (Don't resolve generic impls that may be shadowed by dyn built-in impls)
 - rust-lang#115625 (Explain HRTB + infer limitations of old solver)
 - rust-lang#115839 (Bump libc to 0.2.148)
 - rust-lang#115924 (Don't complain on a single non-exhaustive 1-ZST)
 - rust-lang#115946 (panic when encountering an illegal cpumask in thread::available_parallelism)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 57f1f91 into rust-lang:master Sep 19, 2023
11 checks passed
@rustbot rustbot added this to the 1.74.0 milestone Sep 19, 2023
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Sep 19, 2023
Rollup merge of rust-lang#115946 - the8472:panic-on-sched_getaffinity-bug, r=Mark-Simulacrum

panic when encountering an illegal cpumask in thread::available_parallelism

Fixes rust-lang#115868 by panicking instead of returning an invalid `NonZeroUsize`
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Sep 22, 2023
…-bug, r=cuviper

Fall back to _SC_NPROCESSORS_ONLN if sched_getaffinity returns an empty mask

Followup to rust-lang#115946
A gentler fix for rust-lang#115868, one that doesn't panic, [suggested on zulip](https://rust-lang.zulipchat.com/#narrow/stream/259402-t-libs.2Fmeetings/topic/Meeting.202023-09-19/near/391942927)

In that situation - on the buggy kernel versions - a zero-mask means no affinities have been set so `_SC_NPROCESSORS_ONLN` provides the right value.
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Sep 22, 2023
Rollup merge of rust-lang#116038 - the8472:panic-on-sched_getaffinity-bug, r=cuviper

Fall back to _SC_NPROCESSORS_ONLN if sched_getaffinity returns an empty mask

Followup to rust-lang#115946
A gentler fix for rust-lang#115868, one that doesn't panic, [suggested on zulip](https://rust-lang.zulipchat.com/#narrow/stream/259402-t-libs.2Fmeetings/topic/Meeting.202023-09-19/near/391942927)

In that situation - on the buggy kernel versions - a zero-mask means no affinities have been set so `_SC_NPROCESSORS_ONLN` provides the right value.
@cuviper
Copy link
Member

cuviper commented Sep 22, 2023

@WiSaGaN FYI #116038 gave this a fallback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
O-unix Operating system: Unix-like S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

std::thread::available_parallelism() return 0
6 participants