Small fixes: uninstall target guard, signed/unsigned compare, dead link - #378
Conversation
- CMakeLists.txt: guard the uninstall target with if(NOT TARGET ...). Redefining it breaks configuration when RtMidi is added as a subproject alongside another library that defines its own 'uninstall' target, which is fatal as of CMake 4.1. Fixes thestk#369. - RtMidi.cpp: CreateConnectedEndpointName compared 'int i' against 'size_t nConnected'. Make i a size_t. Fixes thestk#291. - RtMidi.cpp: the mididev Google Groups thread referenced in WinMidiData is no longer publicly reachable. Replace the link with what it explained, keeping the attribution. Fixes thestk#371.
|
Follow-up on verification: the The other two changes (the CMake |
| MidiInApi::MidiMessage message; | ||
| std::vector<LPMIDIHDR> sysexBuffer; | ||
| CRITICAL_SECTION _mutex; // [Patrice] see https://groups.google.com/forum/#!topic/mididev/6OUjHutMpEo | ||
| CRITICAL_SECTION _mutex; // [Patrice] protects the sysex buffer requeue in midiInputCallback |
There was a problem hiding this comment.
I'm not really sure a comment like this is needed. It explains what the mutex is doing, which is obvious from just looking at where the mutex is locked and unlocked. What I was hoping for to find in the dead Google Groups link is the why: How can it be possible that this buffer is accessed from multiple threads, necessitating a mutex? This question remains unanswered.
There was a problem hiding this comment.
The buffer is reachable from two threads because midiInOpen() is called with CALLBACK_FUNCTION, so WinMM invokes midiInputCallback() on a thread it owns, not on any thread the application controls. That callback requeues the sysex buffer with midiInAddBuffer(). Meanwhile MidiInWinMM::closePort() runs on the application's thread and walks the same sysexBuffer vector, calling midiInUnprepareHeader() and freeing each header. Those are the only two places the critical section is taken, and they are the two sides of the race: the driver thread requeueing a buffer that the application thread is in the middle of retiring.
midiInReset() makes it concrete. closePort() calls it while holding the lock, and it is precisely what completes pending buffers and drives the callbacks that then want the same lock. So the mutex is not protecting against two application threads, it is protecting against the driver's own callback thread arriving during teardown.
I could update the comment with something saying that midiInputCallback runs on a WinMM-owned thread and requeues these buffers while closePort retires them from the caller's thread, and this guards that overlap.
Also the surrounding code has active work: #372 fixes a deadlock on exactly this interaction, and #380 rewrites the input teardown for #376.
The comment replacing the dead Google Groups link said what the mutex protects, which is visible from where it is locked. It did not answer the question the link was supposed to answer: how the buffer is reachable from two threads in the first place.
|
Updated the comment, onward and upward. |
2 participants
Three small unrelated fixes, each closing an open issue.
#369 —
add_custom_target(uninstall)is created unconditionally, which fails configuration when RtMidi is added as a subproject alongside another library that defines its ownuninstalltarget. Previously tolerated; fatal as of CMake 4.1. Guarded withif(NOT TARGET ...).#291 —
CreateConnectedEndpointNamedeclaresint iand compares it againstsize_t nConnected. ThenConnectedhalf was already changed tosize_tupstream; this changesito match.#371 — the mididev Google Groups thread referenced in
WinMidiDatais no longer publicly reachable (the group returns an access error). Replaced the link with what it explained, keeping the attribution.Verified on Linux (ALSA + JACK), CMake configure and build clean. The
#291change is in CoreMIDI code, which does not compile on Linux — worth a look from someone on macOS.