Why does the callback for MidiInput::connect need to be 'static?

[kangalio]

The documentation states that the connection to the Midi device is kept open as long as the MidiInputConnection is kept alive. Hence the callback shouldn't be needed anymore once the corresponding MidiInputConnection is dropped. When the callback must only live as long as the MidiInputConnection, the 'static lifetime doesn't make sense. Have I misunderstood something?

[Boddlnagg]

Thanks for pointing this out! When I wrote that part of the code, my knowledge of lifetimes was quite limited, and this was the only way that I could make it compile. I will have a look at it again and see what can be done about it (but it may take a while).
In theory I think you are correct, and I'd readily accept a pull request which changes this.

[kangalio]

I had a look at the source code (src/backend/alsa/mod.rs to be specific) and it seems that a handler thread is spawned for every MidiInputConnection. The callback from MidiInput::connect is moved into that thread which means that the callback has to live for the 'static lifetime (threads created with std::thread::spawn always have static lifetime).

I believe you could use scoped threads as a workaround. Scoped threads are guaranteed to live only for a certain non-'static lifetime and therefore don't require the 'static lifetime. Unfortunately, there is no implementation of it in the standard library, so crossbeam would have to be added as a dependency.

I would say adding that big crate is probably not worth it for this minor issue, what do you think?

[Boddlnagg]

I now remember the reason (as you explained correctly). I don't even think that scoped threads would solve the problem, because what would the scope be? The thread needs to be kept running between method calls, so there's no static scope ...

And I tend to agree that we don't want to pull in another large dependency if the benefit is not very clear.

It might be correct to use some unsafe trickery here, exploiting the knowledge that we have about the lifetime of the thread ... but as long as I don't have a deep understanding of lifetimes of threads/callbacks on the different platforms (I think at least ALSA and Windows would matter here), I don't want to go that route.

[kangalio]

Hi! It's me.
Once again I have the opportunity to use your wonderful library, and once again I am thinking about how to get rid of the 'static lifetime in callbacks :)

So, to get back to the idea of scoped threads.

My idea was to have a scoped thread that guarantees its scope using a guard object of some sort that can be inserted into the midir::MidiInputConnection and will keep care to terminate the thread when dropped. And as a matter of fact, exactly that existed in the Rust standard library at some point. However, it turns out this approach can cause undefined behavior when leaking the scope guard (Rust issue link). For that reason, this API was removed from Rust.

After that, various third-party scoped thread crates popped up, all without any kind of scope guard object - for the sake of safety. Unfortunately, the removal of the scope guard object makes the whole thing unusable for us, because as you say, the thread needs to be kept running between method calls...

As long as we don't leak the scope guard, the original Rust-built-in scoped thread API would have been perfect and safe for our needs. Luckily there exists a stable replacement crate: thread-scoped.

Basically, this is how it works:

let scope = unsafe {
    thread_scoped::scoped(move || {
        // in here, do the midi message polling business
        loop { }
    })
}

// The spawned thread will only run as long as `scope` is kept alive
// So we can just put `scope` into the MidiInputConnection object to ensure that the thread
// doesn't outlive the connection - all is well and safe. (As long as we don't leak the guard
// by doing black magic with Rc's but why should we do that)

The thread-scoped crate has no dependencies whatsoever and as such there's little cost to including it in midir. What do you think?

P.S. Oh yeah I also made a short basic dummy implementation of MidiInputConnection, in order to see if thread-scoped actually works in that case. If you wanna see the dummy implementation with thread-scoped integration, I'll post the link to that too

[kangalio]

I forked midir and made the required modifications in the ALSA backend to remove the 'static constraint on callbacks. It works well and I'm already using the modified version in my project.

The modifications involve adding a lifetime to MidiInputConnectionImpl. As such, common.rs had to be modified to incorporate the lifetime. So in order to stay compatible, all other backends will need to get a lifetime too. I tried to add the lifetime to the JACK backend (that's the only other one I can test locally). I wasn't able to fix all the borrow checker issues though.

As long as only the ALSA backend is compiled, my modifications are already functional though.

I'm looking forward to your thoughts :)

[Boddlnagg]

I'd be interested to see your changes! Maybe you can open a WIP PR (don't worry if it only works/compiles with ALSA)? I will have to see what the addition of a lifetime means for all the other backends.

This might even interact with #25, because the user data is just an additional way to get data into the callback. The way that user data works is that the callback (or rather, the input connection) takes ownership of some data of type T. What if T is borrowed and has a lifetime? Wouldn't that be similar to what you're trying to do? I have to think about this more 😉

[kangalio]

I created the WIP PR as you mentioned: #59.

What if T is borrowed and has a lifetime? Wouldn't that be similar to what you're trying to do?

I don't think this is possible, because T is required to have a static lifetime:

midir/src/common.rs

Lines 155 to 158 in 285a320

	/// Represents an open connection to a MIDI input port.
	pub struct MidiInputConnection<T: 'static> {
	imp: MidiInputConnectionImpl<T>
	}

[Boddlnagg]

I don't think this is possible, because T is required to have a static lifetime:

Yes, but maybe that can be changed? I'm really not sure anymore why it is this way.

[Boddlnagg]

Regarding thread_scope: I looked into your PR (thanks!), and looked into the other backend implementations again ... using thread_scoped will not work with other backends, because they are callback-based. ALSA is really the only backend that manages the input handler thread itself.

CoreMIDI, WinRT and WebMIDI use a callback closure. It might be possible to add a lifetime to that.
Jack and WinMM use an extern "C" fn callback where the user-provided closure is stored in a HandlerData structure.

I tried to add the lifetime to the JACK backend (that's the only other one I can test locally). I wasn't able to fix all the borrow checker issues though.

Yes, that would be the next step. Try to get the JACK (or WinMM) backend working to see whether it's feasible to support this with the other backends. It probably is something that can be done (maybe using a bit of unsafe hackery), but right now I'm not really motivated to put much effort into it. I first want to be sure that it's really worth it and that there's no other (possibly better) approach (e.g. #44 (comment) ? I really don't know ...).

[kangalio]

I'll start another attempt with JACK. Maybe I'll figure it out after spending a bit of time reading the code.

Also, for clarification regarding your idea with user data; you're saying that we could theoretically store the closure's environment in the user data to escape the 'static constraint on the closure? In case I understood that correctly, sure, that seems like a viable workaround.

Though I think that if it's possible to remove the 'static constraint from T, it will be possible to remove the 'static constraint from the closure itself too.

[Boddlnagg]

Regarding the idea about the user data: I think that the user data and the closure environment are really similar conceptually. The main difference is that you can get the user data back out when you close the connection, but I'm not sure if that's even useful.

So I'm still not sure if maybe we should just drop the user data (if the closure environment is sufficient in all cases). I'm still looking for more feedback from others ...

[kangalio]

Look, I managed to fix the JACK backend :) 5683e6d 2 backends done, four to go

The main difference is that you can get the user data back out when you close the connection, but I'm not sure if that's even useful.

Am I misunderstanding or does a capture-based approach nullify the whole purpose behind getting the user data back? When the callback only captures its data, the caller still owns the data and doesn't need to get it back from anything.

let some_value: SomeStruct::new();

midi_input.connect(..., |...| {
    // `some_value` is captured by mutable reference here ...
    some_value.mutate();
}

// ... but ownership still lies in the caller, so the caller doesn't even _need_ to
// get the user data back

[Boddlnagg]

Look, I managed to fix the JACK backend :) 5683e6d 2 backends done, four to go

Great!

Am I misunderstanding or does a capture-based approach nullify the whole purpose behind getting the user data back? When the callback only captures its data, the caller still owns the data and doesn't need to get it back from anything.

Yes, in a sense that's true. If the connect and disconnect (i.e. close) operations happen in the same static scope, it's trivial, but if that's not the case, then you have to arrange things so that your data lives as long as the MidiInputConnection exists and is not moved (which is reflected in the lifetime that you added in your PR). That's probably not too hard to do, but it's a bit harder than just moving the data into connection.

Right now I'm leaning towards the following conclusion:

• If you don't want to worry about the lifetime, you can just move your data into the closure (but you won't be able to get it back)
• If you need to get your data back (which I consider to be a niche use case), you just box it if necessary (so it's not moved) and then borrow it in your closure (which is enabled by your PR)

I think this is sufficiently flexible and allows us to get rid of the user data, thus simplifying the API and resolving #25.

So thanks for moving the discussion around this whole topic forward 👍

[kangalio]

I spun up a Windows 10 VM to be able to fix more backends. Inside the VM I managed to fix the WinMM backend in 9e0adf7.

I also attempted to fix the WinRT backend, however my attempt ultimately failed at the fact that winrt-rust requires a 'static lifetime on the TypedEventHandler callback. See the relevant piece of code:

midir/src/backend/winrt/mod.rs

Lines 109 to 112 in 285a320

	let handler = TypedEventHandler::new(move |_sender, args: *mut MidiMessageReceivedEventArgs| {
	unsafe { MidiInput::handle_input(&*args, &mut *handler_data2.lock().unwrap()) };
	Ok(())
	});

The winrt-rust docs are no help either. TypedEventHandler is barely documented, let alone an explanation on how to get around the 'static restriction. Maybe we'll actually need to use unsafe here and transmute the callback into a 'static-callback in order to trick winrt-rust?

[Boddlnagg]

The WinRT backend will soon be switched from winrt-rust to winrt-rs (see #56). I'm not sure if the situation is better there ... I doubt it. You might be right that we need to throw some unsafe in there ...

[kangalio]

While talking about this on the unofficial Rust Discord server, something was pointed out to me:

Having the callback be 'a instead of 'static is only sound if the MidiInputConnection is closed before 'a goes out of scope. This is usually guaranteed through the Drop impl on MidiInputConnection.

But it's possible to leak values in safe Rust using std::mem::forget():

fn open_and_leak_connection() {
	let local_variable = String::from("This String is a local variable");
	
	let midi_input_connection = midi_input.connect(_, _, |_, _, _| {
		println!("{}", &local_variable);
	}, _).unwrap();

	std::mem::forget(midi_input_connection);
}

fn main() {
	open_and_leak_connection();

	// At this point, the midi connection is still open (it was leaked), but `local_variable` has
	// already been dropped. Therefore, the reference to `local_variable`, which may be used in the
	// midi callback, is dangling now

	std::time::sleep_ms(10000); // Time for midi messages to trickle in and trigger the callback
}

This might be a serious obstacle in our quest to remove the 'static lifetime constraint on callbacks...

[Boddlnagg]

So that is basically the same issue that plagued std::thread::scoped, right? This is sad ...

[Erfa]

This seems to be quite problematic. Is it at all possible to call midi_input.connect() with a callback from a scoped lifetime? I've been trying to connect this to a UI, but any way that isn't directly called from something like main() like all the examples seems to run into this issue in some way. I'm quite new to Rust though, so maybe I'm missing some obvious workaround. Other than this I find this lib very useful!

EDIT: Seems like they key to getting this to work is to send in any state to the callback in an Arc<Mutex<>>. Haven't done a lot of multithreaded things in Rust yet, so it wasn't entirely trivial. This practical example really helped me though if anyone else is finding it difficult: https://github.com/Woyten/tune/blob/master/microwave/src/midi.rs

[kangalio]

To summarize: the callback for MidiInput::connect needs to be 'static, because the MidiInputConnection handler type may be leaked via std::mem::forget() (which is allowed and totally safe in Rust). If MidiInputConnection is leaked, the MIDI connection will never be dropped and will never stop, so the callback will continue to be called for the entire program runtime. Hence, the callback cannot borrow any local variables and must be 'static.