From f61426b86c37f873a52f0440537df8f9a35562a4 Mon Sep 17 00:00:00 2001 From: "Steinar H. Gunderson" Date: Sat, 29 Feb 2020 23:25:11 +0100 Subject: [PATCH] Fix a deadlock in Futatabi when using MIDI devices. If something in the UI wanted to update a light on the MIDI device at the exact same time the operator pressed a button on said device, we could get a deadlock. The problem was that MIDIDevice::handle_event() would lock the MIDIDevice and then go to tell MIDIMapper that the note was coming (which needs a lock on the MIDIMapper), while in the other thread, MIDIMapper::refresh_lights() would first lock the MIDIMapper and then call MIDIDevice::update_lights() with the new set of lights (which nedes a lock on the MIDIDevice). This is a classic lock ordering issue. The solution is to simply make MIDIDevice::handle_event() not lock the MIDIDevice for anything that calls MIDIMapper; it doesn't actually modify any state in MIDIDevice, except when we hotplug new devices (and that never calls MIDIMapper). --- shared/midi_device.cpp | 5 +++-- shared/midi_device.h | 6 ++++-- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/shared/midi_device.cpp b/shared/midi_device.cpp index ab84fda..0383ec2 100644 --- a/shared/midi_device.cpp +++ b/shared/midi_device.cpp @@ -154,7 +154,6 @@ void MIDIDevice::handle_event(snd_seq_t *seq, snd_seq_event_t *event) return; } - lock_guard lock(mu); switch (event->type) { case SND_SEQ_EVENT_CONTROLLER: { receiver->controller_received(event->data.control.param, event->data.control.value); @@ -169,9 +168,11 @@ void MIDIDevice::handle_event(snd_seq_t *seq, snd_seq_event_t *event) receiver->note_on_received(event->data.note.note); break; } - case SND_SEQ_EVENT_PORT_START: + case SND_SEQ_EVENT_PORT_START: { + lock_guard lock(mu); subscribe_to_port_lock_held(seq, event->data.addr); break; + } case SND_SEQ_EVENT_PORT_EXIT: printf("MIDI port %d:%d went away.\n", event->data.addr.client, event->data.addr.port); break; diff --git a/shared/midi_device.h b/shared/midi_device.h index ec6bf9b..5b1c3ad 100644 --- a/shared/midi_device.h +++ b/shared/midi_device.h @@ -60,8 +60,10 @@ private: std::atomic should_quit{false}; int should_quit_fd; - mutable std::recursive_mutex mu; // Recursive because the MIDI receiver may update_lights() back while we are sending it stuff. - MIDIReceiver *receiver; // Under . + // Recursive because the MIDI receiver may update_lights() back while we are sending it stuff. + // TODO: Do we need this anymore after receiver is not under the lock? + mutable std::recursive_mutex mu; + MIDIReceiver *receiver; // _Not_ under ; everything on it should be thread-safe. std::thread midi_thread; std::map current_light_status; // Keyed by note number. Under . -- 2.39.2