Smithay / calloop

A callback-based Event Loop
MIT License
177 stars 34 forks source link

InsertError tends to be non-Send / Sync #193

Closed joshka closed 4 weeks ago

joshka commented 1 month ago

Various errors are non-send / non-sync, which makes them incompatible with color_eyre (without calling map_err and converting the error message to a string). In particular the InsertError when T is non-sync / non-send:

use calloop::{
    timer::{TimeoutAction, Timer},
    EventLoop,
};

fn main() -> color_eyre::Result<()> {
    let event_loop = EventLoop::<App>::try_new()?;
    let timer = Timer::immediate();
    event_loop.handle().insert_source(timer, |_, _, _| {
        println!("Timer expired!");
        TimeoutAction::Drop
    })?;
    Ok(())
}

struct App {}
Compiler Error

``` error[E0277]: `Rc>` cannot be sent between threads safely --> src/main.rs:12:7 | 12 | })?; | ^ `Rc>` cannot be sent between threads safely | = help: within `InsertError`, the trait `Send` is not implemented for `Rc>`, which is required by `Result<(), ErrReport>: FromResidual>>` = help: the following other types implement trait `FromResidual`: as FromResidual>> as FromResidual>> note: required because it appears within the type `timer::Registration` --> /Users/joshka/.cargo/registry/src/index.crates.io-6f17d22bba15001f/calloop-0.14.0/src/sources/timer.rs:39:8 | 39 | struct Registration { | ^^^^^^^^^^^^ note: required because it appears within the type `Option` --> /Users/joshka/.rustup/toolchains/stable-aarch64-apple-darwin/lib/rustlib/src/rust/library/core/src/option.rs:571:10 | 571 | pub enum Option { | ^^^^^^ note: required because it appears within the type `Timer` --> /Users/joshka/.cargo/registry/src/index.crates.io-6f17d22bba15001f/calloop-0.14.0/src/sources/timer.rs:51:12 | 51 | pub struct Timer { | ^^^^^ note: required because it appears within the type `InsertError` --> /Users/joshka/.cargo/registry/src/index.crates.io-6f17d22bba15001f/calloop-0.14.0/src/error.rs:93:12 | 93 | pub struct InsertError { | ^^^^^^^^^^^ = note: required for `ErrReport` to implement `From>` = note: required for `Result<(), ErrReport>` to implement `FromResidual>>` error[E0277]: `Rc>` cannot be shared between threads safely --> src/main.rs:12:7 | 12 | })?; | ^ `Rc>` cannot be shared between threads safely | = help: within `InsertError`, the trait `Sync` is not implemented for `Rc>`, which is required by `Result<(), ErrReport>: FromResidual>>` = help: the following other types implement trait `FromResidual`: as FromResidual>> as FromResidual>> note: required because it appears within the type `timer::Registration` --> /Users/joshka/.cargo/registry/src/index.crates.io-6f17d22bba15001f/calloop-0.14.0/src/sources/timer.rs:39:8 | 39 | struct Registration { | ^^^^^^^^^^^^ note: required because it appears within the type `Option` --> /Users/joshka/.rustup/toolchains/stable-aarch64-apple-darwin/lib/rustlib/src/rust/library/core/src/option.rs:571:10 | 571 | pub enum Option { | ^^^^^^ note: required because it appears within the type `Timer` --> /Users/joshka/.cargo/registry/src/index.crates.io-6f17d22bba15001f/calloop-0.14.0/src/sources/timer.rs:51:12 | 51 | pub struct Timer { | ^^^^^ note: required because it appears within the type `InsertError` --> /Users/joshka/.cargo/registry/src/index.crates.io-6f17d22bba15001f/calloop-0.14.0/src/error.rs:93:12 | 93 | pub struct InsertError { | ^^^^^^^^^^^ = note: required for `ErrReport` to implement `From>` = note: required for `Result<(), ErrReport>` to implement `FromResidual>>` For more information about this error, try `rustc --explain E0277`. error: could not compile `spike-rust` (bin "spike-rust") due to 2 previous errors ```

use calloop::{channel::channel, EventLoop};

fn main() -> color_eyre::Result<()> {
    let event_loop = EventLoop::<App>::try_new()?;
    let (sender, channel) = channel();
    event_loop.handle().insert_source(channel, |_, _, _| {})?;
    Ok(())
}

struct App {}
Compiler Error

``` error[E0277]: `std::sync::mpsc::Receiver<_>` cannot be shared between threads safely --> src/main.rs:6:61 | 6 | event_loop.handle().insert_source(channel, |_, _, _| {})?; | ^ `std::sync::mpsc::Receiver<_>` cannot be shared between threads safely | = help: within `InsertError>`, the trait `Sync` is not implemented for `std::sync::mpsc::Receiver<_>`, which is required by `Result<(), ErrReport>: FromResidual>>>` = help: the following other types implement trait `FromResidual`: as FromResidual>> as FromResidual>> note: required because it appears within the type `Channel<_>` --> /Users/joshka/.cargo/registry/src/index.crates.io-6f17d22bba15001f/calloop-0.14.0/src/sources/channel.rs:123:12 | 123 | pub struct Channel { | ^^^^^^^ note: required because it appears within the type `InsertError>` --> /Users/joshka/.cargo/registry/src/index.crates.io-6f17d22bba15001f/calloop-0.14.0/src/error.rs:93:12 | 93 | pub struct InsertError { | ^^^^^^^^^^^ = note: required for `ErrReport` to implement `From>>` = note: required for `Result<(), ErrReport>` to implement `FromResidual>>>` For more information about this error, try `rustc --explain E0277`. error: could not compile `spike-rust` (bin "spike-rust") due to 1 previous error ```

For the first one, I wonder if converting the RefCell to RwLock makes sense? For the second, I'm not sure what a better thing would be.

It's entirely possible that this is just something that should be ignored and dealt with by mapping the InsertErrors into strings instead of expecting that they can be handled as Send + Sync like this (e.g. .map_err(|e| eyre!("{e}")) and this might be sufficient given insert errors may generally be encountered only really at the beginning of programs (I'm assuming here - might be wrong).

notgull commented 4 weeks ago

InsertError's error field contains the underlying error and is thread safe. So you can do:

result.map_err(|e| e.error)?

Being single threaded is part of calloop's design and is unlikely to change.

joshka commented 4 weeks ago

Ok, that works.

I was curious what the use case for returning the value in the error here was, but there's only one read of this field in all of github and that just boxes the error type and stores it. 🤷‍♂️

Being single threaded is part of calloop's design and is unlikely to change.

Yep, noted. Certainly not advocating to change that.

elinorbgr commented 3 weeks ago

I was curious what the use case for returning the value in the error here was, but there's only one read of this field in all of github and that just boxes the error type and stores it. 🤷‍♂️

The initial intent for that is that, as calloop event sources tend to own underlying resources, giving the source back to the user in case of insertion error allows them to decide what to do with them, instead of unconditionally dropping the source (and likely closing/freeing the resource).

This is maybe a niche need, but it seems to me that support it that is still worth it, given it is very easy to ignore the returned source if you don't care about it (using map_err).

joshka commented 3 weeks ago

Makes sense - thanks for that.