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).

diff --git a/shared/midi_device.cpp b/shared/midi_device.cpp
index ab84fda6db51099e3ecf03013bc08a7b0a60947a..0383ec20d921bd8166595a24f4fe6fe3201124a3 100644 (file)
--- 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<recursive_mutex> 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<recursive_mutex> 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 ec6bf9b5a160b5ededf14600744435a96ef0e951..5b1c3ade4c4ea8c884f9b131f5caa735b082268c 100644 (file)
--- a/shared/midi_device.h
+++ b/shared/midi_device.h
@@ -60,8 +60,10 @@ private:
        std::atomic<bool> 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 <mu>.
+       // 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 <mu>; everything on it should be thread-safe.
 
        std::thread midi_thread;
        std::map<LightKey, uint8_t> current_light_status;  // Keyed by note number. Under <mu>.