Client is not Send

[Boddlnagg]

Is it intentional that coremidi::Client is not Send?

    = help: within `common::MidiInput`, the trait `std::marker::Send` is not implemented for `*mut std::boxed::Box<(dyn for<'r> std::ops::FnMut(&'r backend::coremidi::coremidi::Notification) + 'static)>`
    = note: required because it appears within the type `backend::coremidi::coremidi::BoxedCallback<backend::coremidi::coremidi::Notification>`
    = note: required because it appears within the type `backend::coremidi::coremidi::Client`
    = note: required because it appears within the type `backend::coremidi::MidiInput`

I was not able to find any information regarding the thread safety of MIDIClientRef, but I also don't know about thread-safety in the macOS APIs in general ...

I had users of midir complain that some structs are not Send, so I'm trying to implement Send for everything, but of course I want to know whether it's actually safe ...

[chris-zen]

Hi @Boddlnagg, when I implemented this lib for the first time, I was just onboarding that side of rust. I think that it would require a re-visit. I might look at how PortAudio handles this case and also read about thread safety for the CoreMIDI SDK.
As you can see, my response time is quite sparse, so please let me know what your expectations would be around this work, and whether you could help me or not, so I can plan myself accordingly ;-)

[Boddlnagg]

Hi @chris-zen, thanks for your reply, and I understand that you don't have a lot of spare time for this project. It's fine.

I think the implementation would be trivial once it's clear whether marking Client as Send is actually safe. From my own (limited) understanding, I would guess that it would be safe, because Send just means that you would be able to use a MIDIClientRef from a different thread than the one you created it on. This is a much weaker guarantee than saying that it's safe to use from multiple threads at the same time concurrently.

I haven't been able to find documentation about this so far. I only found this, which claims that the CoreMIDI API is not thread-safe at all, and any call to a CoreMIDI API needs to be synchronized by the caller. But I can't really believe that (I would expect that separate MIDIClientRefs could at least be used concurrently within the respective thread that created them). If that statement were true, then this crate would already be unsafe because no one prevents the user from calling coremidi::Client::new from different threads concurrently.

It would be great to find someone who knows more about all this ... the public documentation doesn't really help much, unfortunately.

It probably won't help much to look into other CoreMIDI wrappers (such as PortMidi), because the guarantee that Send makes is one that is usually not made explicit anywhere (in other languages apart from Rust). It might just be that we find that this guarantee is implicitly relied upon in some library code, which would strengthen my confidence that it's safe/correct.

[chris-zen]

Hi @Boddlnagg, after reading the CoreMIDI documentation from Apple, I found this sentence here that makes me think that we are safe to use Send:

The Core MIDI framework uses IPC (interprocess communication) to communicate with a server process, the MIDI server. The server process in turn loads, and manages all communication with, the MIDI driver.

I interpret it as the clients just getting opaque handlers to objects created and managed in a central process (the CoreMIDI server), where every call from the client translates into IPC messages to it. My assumption is that it doesn't matter from what process or thread do the messages come from, as the server will take care of their ordering and dispatching.
So I personally don't see any problem to use Send.
Now the question is what changes are required in this crate. Do you have a clear idea ? are you willing to prepare a PR ? or at least to tell me which objects require Send ?

[Boddlnagg]

This sounds great!

The specific errors that I get are this:

error[E0277]: `*mut std::boxed::Box<(dyn for<'r> std::ops::FnMut(&'r backend::coremidi::coremidi::Notification) + 'static)>` cannot be sent between threads safely
   --> src/common.rs:242:9
    |
242 |         is_send::<MidiInput>();
    |         ^^^^^^^^^^^^^^^^^^^^ `*mut std::boxed::Box<(dyn for<'r> std::ops::FnMut(&'r backend::coremidi::coremidi::Notification) + 'static)>` cannot be sent between threads safely
    |
    = help: within `common::MidiInput`, the trait `std::marker::Send` is not implemented for `*mut std::boxed::Box<(dyn for<'r> std::ops::FnMut(&'r backend::coremidi::coremidi::Notification) + 'static)>`
    = note: required because it appears within the type `backend::coremidi::coremidi::BoxedCallback<backend::coremidi::coremidi::Notification>`
    = note: required because it appears within the type `backend::coremidi::coremidi::Client`
    = note: required because it appears within the type `backend::coremidi::MidiInput`
    = note: required because it appears within the type `common::MidiInput`
note: required by `common::tests::test_trait_impls::is_send`
   --> src/common.rs:240:9
    |
240 |         fn is_send<T: Send>() {}
    |         ^^^^^^^^^^^^^^^^^^^^^

error[E0277]: `*mut std::boxed::Box<(dyn for<'r> std::ops::FnMut(&'r backend::coremidi::coremidi::Notification) + 'static)>` cannot be sent between threads safely
   --> src/common.rs:243:9
    |
243 |         is_send::<MidiInputConnection<()>>();
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `*mut std::boxed::Box<(dyn for<'r> std::ops::FnMut(&'r backend::coremidi::coremidi::Notification) + 'static)>` cannot be sent between threads safely
    |
    = help: within `common::MidiInputConnection<()>`, the trait `std::marker::Send` is not implemented for `*mut std::boxed::Box<(dyn for<'r> std::ops::FnMut(&'r backend::coremidi::coremidi::Notification) + 'static)>`
    = note: required because it appears within the type `backend::coremidi::coremidi::BoxedCallback<backend::coremidi::coremidi::Notification>`
    = note: required because it appears within the type `backend::coremidi::coremidi::Client`
    = note: required because it appears within the type `backend::coremidi::MidiInputConnection<()>`
    = note: required because it appears within the type `common::MidiInputConnection<()>`
note: required by `common::tests::test_trait_impls::is_send`
   --> src/common.rs:240:9
    |
240 |         fn is_send<T: Send>() {}
    |         ^^^^^^^^^^^^^^^^^^^^^

error[E0277]: `*mut std::boxed::Box<(dyn for<'r> std::ops::FnMut(&'r backend::coremidi::coremidi::PacketList) + 'static)>` cannot be sent between threads safely
   --> src/common.rs:243:9
    |
243 |         is_send::<MidiInputConnection<()>>();
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `*mut std::boxed::Box<(dyn for<'r> std::ops::FnMut(&'r backend::coremidi::coremidi::PacketList) + 'static)>` cannot be sent between threads safely
    |
    = help: within `common::MidiInputConnection<()>`, the trait `std::marker::Send` is not implemented for `*mut std::boxed::Box<(dyn for<'r> std::ops::FnMut(&'r backend::coremidi::coremidi::PacketList) + 'static)>`
    = note: required because it appears within the type `backend::coremidi::coremidi::BoxedCallback<backend::coremidi::coremidi::PacketList>`
    = note: required because it appears within the type `backend::coremidi::coremidi::InputPort`
    = note: required because it appears within the type `backend::coremidi::InputConnectionDetails`
    = note: required because it appears within the type `backend::coremidi::MidiInputConnection<()>`
    = note: required because it appears within the type `common::MidiInputConnection<()>`
note: required by `common::tests::test_trait_impls::is_send`
   --> src/common.rs:240:9
    |
240 |         fn is_send<T: Send>() {}
    |         ^^^^^^^^^^^^^^^^^^^^^

error[E0277]: `*mut std::boxed::Box<(dyn for<'r> std::ops::FnMut(&'r backend::coremidi::coremidi::Notification) + 'static)>` cannot be sent between threads safely
   --> src/common.rs:245:9
    |
245 |         is_send::<MidiOutput>();
    |         ^^^^^^^^^^^^^^^^^^^^^ `*mut std::boxed::Box<(dyn for<'r> std::ops::FnMut(&'r backend::coremidi::coremidi::Notification) + 'static)>` cannot be sent between threads safely
    |
    = help: within `common::MidiOutput`, the trait `std::marker::Send` is not implemented for `*mut std::boxed::Box<(dyn for<'r> std::ops::FnMut(&'r backend::coremidi::coremidi::Notification) + 'static)>`
    = note: required because it appears within the type `backend::coremidi::coremidi::BoxedCallback<backend::coremidi::coremidi::Notification>`
    = note: required because it appears within the type `backend::coremidi::coremidi::Client`
    = note: required because it appears within the type `backend::coremidi::MidiOutput`
    = note: required because it appears within the type `common::MidiOutput`
note: required by `common::tests::test_trait_impls::is_send`
   --> src/common.rs:240:9
    |
240 |         fn is_send<T: Send>() {}
    |         ^^^^^^^^^^^^^^^^^^^^^

error: aborting due to 4 previous errors

This means that coremidi::Client and coremidi::InputPort need to be Send.

You can easily test this by just defining a simple function and calling it:

fn is_send<T: Send>() {}
is_send::<Client>();
is_send::<InputPort>();

However, after looking at the above errors again, I realize that the problem might not actually be the Core MIDI types, but the BoxedCallback. Since the callback function in BoxedCallback::new requires a Send bound, I think that BoxedCallback itself can also be made Send, simply by unsafe impl<T> Send for BoxedCallback<T> {}, but I'm not 100% sure ...

[chris-zen]

I see you are available now. I also have some time available now. Would you like we to pair and try to implement this together ? or else could you tell me what code are you compiling so I can reproduce those errors ?

[Boddlnagg]

I'm not really available, but I can point you to the code:
The build that's failing is this Pull Request: Boddlnagg/midir#38 (see also https://dev.azure.com/Boddlnagg/b5ee628a-e54a-4a1c-90af-0a24d4ecd7d6/_build/results?buildId=25)

It's just asserting that midir::MidiInput is Send, which currently fails for the coremidi backend.

[Boddlnagg]

Looking into this again, because I'm still bitten by it ... (Boddlnagg/midir#38 fails to build)
I think my previous assesment is correct: All that's needed is a

unsafe impl<T> Send for BoxedCallback<T> {}

in lib.rs (which is safe because there's a F: Send bound on the boxed function).

I would open a pull request if I had a macOS machine to try it ...

[chris-zen]

@Boddlnagg please, could you review #15 ?
Please note that Travis takes care of the build for MacOS, so you don't need to have a MacOS to test things ;-)

[chris-zen]

@Boddlnagg
coremidi = "0.4.0"
😉