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

Condvar isn't RefUnwindSafe when building on Mac #118009

Closed
ecton opened this issue Nov 17, 2023 · 10 comments · Fixed by #121768
Closed

Condvar isn't RefUnwindSafe when building on Mac #118009

ecton opened this issue Nov 17, 2023 · 10 comments · Fixed by #121768
Labels
C-bug Category: This is a bug. O-macos Operating system: macOS T-libs Relevant to the library team, which will review and decide on the PR/issue.

Comments

@ecton
Copy link
Contributor

ecton commented Nov 17, 2023

use std::sync::{Condvar, Mutex};
fn panic_catcher() {
    let condvar = Condvar::new();
    std::panic::catch_unwind(move || {
        let mutex = Mutex::new(());
        let guard = mutex.lock().unwrap();
        condvar.wait(guard);
    });
}

I expected to see this happen: No errors. This code compiles on Linux, as I would expect based on Condvar's documentation.

Instead, this happened: When compiling on Mac OS, I get this error:

error[E0277]: the type `UnsafeCell<libc::unix::bsd::apple::pthread_cond_t>` may contain interior mutability and a reference may not be safely transferrable across a catch_unwind boundary
    --> src/lib.rs:176:30
     |
176  |       std::panic::catch_unwind(move || {
     |  _____------------------------_^
     | |     |
     | |     required by a bound introduced by this call
177  | |         let mutex = Mutex::new(());
178  | |         let guard = mutex.lock().unwrap();
179  | |         condvar.wait(guard);
180  | |     });
     | |_____^ `UnsafeCell<libc::unix::bsd::apple::pthread_cond_t>` may contain interior mutability and a reference may not be safely transferrable across a catch_unwind boundary
     |
     = help: within `std::sys::unix::locks::pthread_condvar::AllocatedCondvar`, the trait `RefUnwindSafe` is not implemented for `UnsafeCell<libc::unix::bsd::apple::pthread_cond_t>`
note: required because it appears within the type `AllocatedCondvar`
    --> /Users/ecton/.rustup/toolchains/stable-aarch64-apple-darwin/lib/rustlib/src/rust/library/std/src/sys/unix/locks/pthread_condvar.rs:12:8
     |
12   | struct AllocatedCondvar(UnsafeCell<libc::pthread_cond_t>);
     |        ^^^^^^^^^^^^^^^^
     = note: required for `*mut std::sys::unix::locks::pthread_condvar::AllocatedCondvar` to implement `UnwindSafe`
note: required because it appears within the type `UnsafeCell<*mut AllocatedCondvar>`
    --> /Users/ecton/.rustup/toolchains/stable-aarch64-apple-darwin/lib/rustlib/src/rust/library/core/src/cell.rs:1987:12
     |
1987 | pub struct UnsafeCell<T: ?Sized> {
     |            ^^^^^^^^^^
note: required because it appears within the type `AtomicPtr<AllocatedCondvar>`
    --> /Users/ecton/.rustup/toolchains/stable-aarch64-apple-darwin/lib/rustlib/src/rust/library/core/src/sync/atomic.rs:186:12
     |
186  | pub struct AtomicPtr<T> {
     |            ^^^^^^^^^
note: required because it appears within the type `LazyBox<AllocatedCondvar>`
    --> /Users/ecton/.rustup/toolchains/stable-aarch64-apple-darwin/lib/rustlib/src/rust/library/std/src/sys_common/lazy_box.rs:13:19
     |
13   | pub(crate) struct LazyBox<T: LazyInit> {
     |                   ^^^^^^^
note: required because it appears within the type `Condvar`
    --> /Users/ecton/.rustup/toolchains/stable-aarch64-apple-darwin/lib/rustlib/src/rust/library/std/src/sys/unix/locks/pthread_condvar.rs:14:12
     |
14   | pub struct Condvar {
     |            ^^^^^^^
note: required because it appears within the type `Condvar`
    --> /Users/ecton/.rustup/toolchains/stable-aarch64-apple-darwin/lib/rustlib/src/rust/library/std/src/sync/condvar.rs:107:12
     |
107  | pub struct Condvar {
     |            ^^^^^^^
note: required because it's used within this closure
    --> src/lib.rs:176:30
     |
176  |     std::panic::catch_unwind(move || {
     |                              ^^^^^^^
note: required by a bound in `catch_unwind`
    --> /Users/ecton/.rustup/toolchains/stable-aarch64-apple-darwin/lib/rustlib/src/rust/library/std/src/panic.rs:141:40
     |
141  | pub fn catch_unwind<F: FnOnce() -> R + UnwindSafe, R>(f: F) -> Result<R> {
     |                                        ^^^^^^^^^^ required by this bound in `catch_unwind`

On Linux, this code compiles correctly, and the hosted Rust docs for Convdar document this type as UnwindSafe. I'm filing this issue to ask:

Is the current MacOS implementation not actually UnwindSafe? My workaround for this issue is to wrap Condvar in AssertUnwindwSafe, but I would like to confirm this is actually safe. My personal hope is that these sync primitives are UnwindSafe and that this is a safe workaround.

Meta

rustc --version --verbose:

rustc 1.74.0 (79e9716c9 2023-11-13)
binary: rustc
commit-hash: 79e9716c980570bfd1f666e3b16ac583f0168962
commit-date: 2023-11-13
host: x86_64-unknown-linux-gnu
release: 1.74.0
LLVM version: 17.0.4
@ecton ecton added the C-bug Category: This is a bug. label Nov 17, 2023
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Nov 17, 2023
@Jules-Bertholet
Copy link
Contributor

@rustbot label O-macos T-libs

@rustbot rustbot added O-macos Operating system: macOS T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Nov 17, 2023
ecton added a commit to khonsulabs/cushy that referenced this issue Nov 17, 2023
After trying to run Gooey again on my Mac for the first time in a few
weeks, I found that I ran into the Condvar issue again. Rather than
pasting AssertUnwindSafe in those files, I've both reported the
discrepency in unwind safety (rust-lang/rust#118009) and moved the
workaround into a type that only uses AssertUnwindsafe when compiling
for Apple.
@Noratrieb
Copy link
Member

sounds like a bug, there's probably an implementation missing on some MacOS specific sys struct, the unwind safety traits are always forgotten
feel free to send a PR adding it

@Noratrieb Noratrieb removed the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Nov 17, 2023
@fzyzcjy
Copy link

fzyzcjy commented Dec 2, 2023

Hi, is there any updates? I am seeing the same issue.

@Noratrieb
Copy link
Member

No updates because no one has fixed this yet. Feel free to make a PR and fix if yourself :)

@ecton
Copy link
Contributor Author

ecton commented Dec 3, 2023

Is it correct to assume that Condvar is supposed to be UnwindSafe on all platforms? My basic understanding of this is that Condvar should be UnwindSafe and, barring any surprises, the data structures should already be unwind safe and are simply not marked as such.

it seems like this issue likely affects multiple platforms, as similar primitives are used across other sys Condvars without accompanying UnwindSafe impls.

I'm happy to try to put together a PR that adds a test asserting unwind safety (similar tests exist for other data structures) if someone more knowledgeable than me can confirm it is the correct thing to do. This would be my first contribution, which is also why I'm hesitant to throw in a PR about something I'm not sure I fully understand.

ecton added a commit to khonsulabs/cushy that referenced this issue Dec 19, 2023
Condvar isn't UnwindSafe on Windows either. See
rust-lang/rust#118009
@tomasstranik
Copy link

What's a current state of this bug, please?

@Noratrieb
Copy link
Member

@ecton said they'd work on here, but nothing seems to have happened. If you want to, you can submit a PR fixing it.

@ecton
Copy link
Contributor Author

ecton commented Feb 23, 2024

@ecton said they'd work on here, but nothing seems to have happened. If you want to, you can submit a PR fixing it.

Actually the current state is asking whether it is the correct behavior for all platforms or not. My last comment expressed how I do not have confidence to know that it's the right choice, and was looking for confirmation on the approach before beginning work.

@Noratrieb
Copy link
Member

Generally for these platforms, we would really like them all to be the same, otherwise it results in really confusing issues.

@tomasstranik
Copy link

I do agree.
A condition variable's behavior as a synchronization primitive should be consistent across platforms. If it is UnwindSafe on Linux, it should also be on macOS.

jhpratt added a commit to jhpratt/rust that referenced this issue Feb 29, 2024
Implement unwind safety for Condvar on all platforms

Closes rust-lang#118009

This commit adds unwind safety consistency to Condvar. Previously, only select platforms implemented unwind safety through auto traits. Known by this committer: On Linux, `Condvar` implemented `UnwindSafe` but on Mac and Windows, it did not. This change changes the implementation from auto to explicit.

In rust-lang#118009, it was suggested that the platform differences were a bug and that a simple PR could address this. In trying to determine the best information to put in the `#[stable]` attribute, it [was suggested](rust-lang#121690 (comment)) I copy the stability information from the previous unwind safety implementations.
jhpratt added a commit to jhpratt/rust that referenced this issue Feb 29, 2024
Implement unwind safety for Condvar on all platforms

Closes rust-lang#118009

This commit adds unwind safety consistency to Condvar. Previously, only select platforms implemented unwind safety through auto traits. Known by this committer: On Linux, `Condvar` implemented `UnwindSafe` but on Mac and Windows, it did not. This change changes the implementation from auto to explicit.

In rust-lang#118009, it was suggested that the platform differences were a bug and that a simple PR could address this. In trying to determine the best information to put in the `#[stable]` attribute, it [was suggested](rust-lang#121690 (comment)) I copy the stability information from the previous unwind safety implementations.
@bors bors closed this as completed in 5512945 Feb 29, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Feb 29, 2024
Rollup merge of rust-lang#121768 - ecton:condvar-unwindsafe, r=m-ou-se

Implement unwind safety for Condvar on all platforms

Closes rust-lang#118009

This commit adds unwind safety consistency to Condvar. Previously, only select platforms implemented unwind safety through auto traits. Known by this committer: On Linux, `Condvar` implemented `UnwindSafe` but on Mac and Windows, it did not. This change changes the implementation from auto to explicit.

In rust-lang#118009, it was suggested that the platform differences were a bug and that a simple PR could address this. In trying to determine the best information to put in the `#[stable]` attribute, it [was suggested](rust-lang#121690 (comment)) I copy the stability information from the previous unwind safety implementations.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. O-macos Operating system: macOS T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants