Errors returned by connect() aren't Sync

[Deewiant]

I was trying to use https://github.com/dtolnay/anyhow with midir's errors and ran into this rather quickly: The ConnectError<MidiInput> and ConnectError<MidiOutput> types don't implement Sync (at least with ALSA), which makes them incompatible with e.g. anyhow's Error trait.

This can be demonstrated by tweaking the tests in this silly way:

diff --git tests/virtual.rs tests/virtual.rs
index 5ac5754..4dbc2ef 100644
--- tests/virtual.rs
+++ tests/virtual.rs
@@ -8,6 +8,8 @@ use std::time::Duration;
 use midir::{MidiInput, MidiOutput, Ignore, MidiOutputPort};
 use midir::os::unix::{VirtualInput, VirtualOutput};
 
+fn is_sync<T: Sync>(t: T) -> ! { panic!() }
+
 #[test]
 fn end_to_end() {
     let mut midi_in = MidiInput::new("My Test Input").unwrap();
@@ -26,7 +28,7 @@ fn end_to_end() {
     let new_port: MidiOutputPort = midi_out.ports().into_iter().rev().next().unwrap();
 
     println!("Connecting to port '{}' ...", midi_out.port_name(&new_port).unwrap());
-    let mut conn_out = midi_out.connect(&new_port, "midir-test").unwrap();
+    let mut conn_out = match midi_out.connect(&new_port, "midir-test") { Ok(c) => c, Err(e) => is_sync(e) };
     println!("Starting to send messages ...");
     conn_out.send(&[144, 60, 1]).unwrap();
     sleep(Duration::from_millis(200));

cargo test then complains:

error[E0277]: `*mut alsa_sys::snd_seq_t` cannot be shared between threads safely
  --> tests/virtual.rs:31:96
   |
11 | fn is_sync<T: Sync>(t: T) -> ! { panic!() }
   |    -------    ---- required by this bound in `is_sync`
...
31 |     let mut conn_out = match midi_out.connect(&new_port, "midir-test") { Ok(c) => c, Err(e) => is_sync(e) };
   |                                                                                                ^^^^^^^ `*mut alsa_sys::snd_seq_t` cannot be shared between threads safely
   |
   = help: within `midir::ConnectError<midir::MidiOutput>`, the trait `std::marker::Sync` is not implemented for `*mut alsa_sys::snd_seq_t`
   = note: required because it appears within the type `alsa::seq::Seq`
   = note: required because it appears within the type `std::option::Option<alsa::seq::Seq>`
   = note: required because it appears within the type `midir::backend::alsa::MidiOutput`
   = note: required because it appears within the type `midir::MidiOutput`
   = note: required because it appears within the type `midir::ConnectError<midir::MidiOutput>`

error[E0277]: `std::cell::Cell<bool>` cannot be shared between threads safely
  --> tests/virtual.rs:31:96
   |
11 | fn is_sync<T: Sync>(t: T) -> ! { panic!() }
   |    -------    ---- required by this bound in `is_sync`
...
31 |     let mut conn_out = match midi_out.connect(&new_port, "midir-test") { Ok(c) => c, Err(e) => is_sync(e) };
   |                                                                                                ^^^^^^^ `std::cell::Cell<bool>` cannot be shared between threads safely
   |
   = help: within `midir::ConnectError<midir::MidiOutput>`, the trait `std::marker::Sync` is not implemented for `std::cell::Cell<bool>`
   = note: required because it appears within the type `alsa::seq::Seq`
   = note: required because it appears within the type `std::option::Option<alsa::seq::Seq>`
   = note: required because it appears within the type `midir::backend::alsa::MidiOutput`
   = note: required because it appears within the type `midir::MidiOutput`
   = note: required because it appears within the type `midir::ConnectError<midir::MidiOutput>`

error: aborting due to 2 previous errors

I guess the intention with these error types is that connect consumes the original MidiInput/MidiOutput, but logically only does so if it succeeds, so including it in the error provides a means of getting it back on failure. That makes sense, but it does lead to this kind of papercut.

This can be worked around by doing something like ConnectError::new(e.kind(), ()), or converting the error to a string immediately, but it's a bit awkward.

The best idea off the top of my head would be to offer an API for disregarding the "inner" value in these errors, which would streamline the workaround a bit. But maybe there's a cleaner solution.

This really is just a papercut in the API, so it's not a big deal. But it's a bit non-obvious and maybe even non-intentional, so I figured I'd raise it here.

[Boddlnagg]

This really is just a papercut in the API, so it's not a big deal. But it's a bit non-obvious and maybe even non-intentional, so I figured I'd raise it here.

Yes, it's actually not intentional, so thanks for raising it!

The best idea off the top of my head would be to offer an API for disregarding the "inner" value in these errors, which would streamline the workaround a bit. But maybe there's a cleaner solution.

I don't have a better idea right now, either ... I need to think about it little more.

[Boscop]

@Deewiant I'm trying to build my project for linux and running into this issue with alsa, did you find a workaround?
@Boddlnagg Or do you have an idea for a fix? :)
I want to keep using anyhow if possible.

error[E0277]: `*mut alsa_sys::_snd_seq` cannot be shared between threads safely
  --> vmidi/src/midiport.rs:51:29
   |
51 |         /*.map_err(|e| e.kind())*/?,
   |                                   ^ `*mut alsa_sys::_snd_seq` cannot be shared between threads safely
   |
   = help: within `ConnectError<MidiInput>`, the trait `Sync` is not implemented for `*mut alsa_sys::_snd_seq`
   = note: required because it appears within the type `alsa::seq::Seq`
   = note: required because it appears within the type `std::option::Option<alsa::seq::Seq>`
   = note: required because it appears within the type `midir::backend::alsa::MidiInput`
   = note: required because it appears within the type `MidiInput`
   = note: required because it appears within the type `ConnectError<MidiInput>`
   = note: required because of the requirements on the impl of `From<ConnectError<MidiInput>>` for `anyhow::Error`
   = note: required by `std::convert::From::from`

error[E0277]: `Cell<bool>` cannot be shared between threads safely
  --> vmidi/src/midiport.rs:51:29
   |
51 |         /*.map_err(|e| e.kind())*/?,
   |                                   ^ `Cell<bool>` cannot be shared between threads safely
   |
   = help: within `ConnectError<MidiInput>`, the trait `Sync` is not implemented for `Cell<bool>`
   = note: required because it appears within the type `alsa::seq::Seq`
   = note: required because it appears within the type `std::option::Option<alsa::seq::Seq>`
   = note: required because it appears within the type `midir::backend::alsa::MidiInput`
   = note: required because it appears within the type `MidiInput`
   = note: required because it appears within the type `ConnectError<MidiInput>`
   = note: required because of the requirements on the impl of `From<ConnectError<MidiInput>>` for `anyhow::Error`
   = note: required by `std::convert::From::from`

error[E0277]: `*mut alsa_sys::_snd_seq` cannot be shared between threads safely
  --> vmidi/src/midiport.rs:80:54
   |
80 |     Ok(midi.connect(&port, "")/*.map_err(|e| e.kind())*/?)
   |                                                         ^ `*mut alsa_sys::_snd_seq` cannot be shared between threads safely
   |
   = help: within `ConnectError<MidiOutput>`, the trait `Sync` is not implemented for `*mut alsa_sys::_snd_seq`
   = note: required because it appears within the type `alsa::seq::Seq`
   = note: required because it appears within the type `std::option::Option<alsa::seq::Seq>`
   = note: required because it appears within the type `midir::backend::alsa::MidiOutput`
   = note: required because it appears within the type `MidiOutput`
   = note: required because it appears within the type `ConnectError<MidiOutput>`
   = note: required because of the requirements on the impl of `From<ConnectError<MidiOutput>>` for `anyhow::Error`
   = note: required by `std::convert::From::from`

error[E0277]: `Cell<bool>` cannot be shared between threads safely
  --> vmidi/src/midiport.rs:80:54
   |
80 |     Ok(midi.connect(&port, "")/*.map_err(|e| e.kind())*/?)
   |                                                         ^ `Cell<bool>` cannot be shared between threads safely
   |
   = help: within `ConnectError<MidiOutput>`, the trait `Sync` is not implemented for `Cell<bool>`
   = note: required because it appears within the type `alsa::seq::Seq`
   = note: required because it appears within the type `std::option::Option<alsa::seq::Seq>`
   = note: required because it appears within the type `midir::backend::alsa::MidiOutput`
   = note: required because it appears within the type `MidiOutput`
   = note: required because it appears within the type `ConnectError<MidiOutput>`
   = note: required because of the requirements on the impl of `From<ConnectError<MidiOutput>>` for `anyhow::Error`
   = note: required by `std::convert::From::from`

[Deewiant]

@Boscop The workaround I mentioned should work, i.e. throwing away the MidiOutput in the error, replacing it with ():

Ok(midi.connect(&port, "").map_err(|e| ConnectError::new(e.kind(), ()))?)

[Boscop]

@Deewiant But doesn't that lose information about the error? The details?

[Deewiant]

@Boscop You only lose the MidiOutput you were trying to connect, which probably isn't very important to you by the time you're using ? to propagate the error out? Depends on your application I guess. The ConnectError itself doesn't do anything with it other than carry it around.