coremidi: fix hang on shutdown and on quick restart

macosx/coremidi/JackCoreMidiDriver.h

@@ -27,6 +27,7 @@ Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
 #include "JackCoreMidiVirtualOutputPort.h"
 #include "JackMidiDriver.h"
 #include "JackThread.h"
+#include <atomic>
 
 namespace Jack {
 
@@ -63,6 +64,9 @@ namespace Jack {
         void Restart();
 
         JackThread fThread;    /*! Thread to execute the Process function */
+        CFRunLoopRef fRunLoop; // run loop of fThread
+        std::atomic<bool> fClosing; // set by Close(), makes fThread exit
+        std::atomic<bool> fStarted; // true once Start() is done
 
     public:
 

macosx/coremidi/JackCoreMidiDriver.mm

@@ -63,7 +63,7 @@
 JackCoreMidiDriver::JackCoreMidiDriver(const char *name, const char *alias,
                                        JackLockedEngine *engine,
                                        JackSynchro *table):
-    JackMidiDriver(name, alias, engine, table),fThread(this)
+    JackMidiDriver(name, alias, engine, table),fThread(this),fRunLoop(NULL),fClosing(false),fStarted(false)
 {
     mach_timebase_info_data_t info;
     kern_return_t result = mach_timebase_info(&info);
@@ -91,6 +91,8 @@
 
 bool JackCoreMidiDriver::Init()
 {
+    // needed to stop the run loop in Close()
+    fRunLoop = CFRunLoopGetCurrent();
     return OpenAux();
 }
 
@@ -340,7 +342,14 @@ new JackCoreMidiVirtualOutputPort(fAliasName, client_name,
 
 bool JackCoreMidiDriver::Execute()
 {
-    CFRunLoopRun();
+    // CFRunLoopRun() cannot be cancelled, run in short slices and check fClosing
+    while (!fClosing) {
+        SInt32 result = CFRunLoopRunInMode(kCFRunLoopDefaultMode, 0.1, false);
+        if (result == kCFRunLoopRunFinished) {
+            // nothing attached to the run loop, do not spin
+            JackSleep(100000);
+        }
+    }
     return false;
 }
 
@@ -451,7 +460,13 @@ new JackCoreMidiVirtualOutputPort(fAliasName, client_name,
 int
 JackCoreMidiDriver::Close()
 {
-    fThread.Kill();
+    // Kill() cannot interrupt the run loop, stop it and wait for the thread
+    fClosing = true;
+    if (fRunLoop) {
+        CFRunLoopStop(fRunLoop);
+    }
+    fThread.Stop();
+    fRunLoop = NULL;
     return CloseAux();
 }
 
@@ -552,6 +567,11 @@ new JackCoreMidiVirtualOutputPort(fAliasName, client_name,
 void
 JackCoreMidiDriver::HandleNotification(const MIDINotification *message)
 {
+    // ignore changes while closing, and while starting as a restart would race with Attach()
+    if (fClosing || !fStarted) {
+        return;
+    }
+
     switch (message->messageID) {
 
         case kMIDIMsgObjectAdded: {
@@ -598,6 +618,7 @@ new JackCoreMidiVirtualOutputPort(fAliasName, client_name,
     capture_latency = capture_latency_aux;
     playback_latency = playback_latency_aux;
 
+    fClosing = false;
     fThread.StartSync();
 
     int count = 0;
@@ -672,6 +693,7 @@ new JackCoreMidiVirtualOutputPort(fAliasName, client_name,
 
     jack_info("JackCoreMidiDriver::Start - Driver started.");
 
+    fStarted = true;
     return 0;
 
  stop_virtual_output_ports:
@@ -711,6 +733,7 @@ new JackCoreMidiVirtualOutputPort(fAliasName, client_name,
 {
     int result = 0;
 
+    fStarted = false;
     JackMidiDriver::Stop();
 
     jack_info("JackCoreMidiDriver::Stop - disabling physical input ports.");

macosx/coremidi/JackCoreMidiOutputPort.h

@@ -20,6 +20,7 @@ Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
 #ifndef __JackCoreMidiOutputPort__
 #define __JackCoreMidiOutputPort__
 
+#include <atomic>
 #include <semaphore.h>
 
 #include "JackCoreMidiPort.h"
@@ -49,6 +50,7 @@ namespace Jack {
         JackThread *thread;
         JackMidiAsyncQueue *thread_queue;
         sem_t *thread_queue_semaphore;
+        std::atomic<bool> stopping; // set by Stop(), makes the thread exit
 
     protected:
 

macosx/coremidi/JackCoreMidiOutputPort.mm

@@ -33,7 +33,7 @@
 JackCoreMidiOutputPort::JackCoreMidiOutputPort(double time_ratio,
                                                size_t max_bytes,
                                                size_t max_messages):
-    JackCoreMidiPort(time_ratio)
+    JackCoreMidiPort(time_ratio), stopping(false)
 {
     read_queue = new JackMidiBufferReadQueue();
     std::unique_ptr<JackMidiBufferReadQueue> read_queue_ptr(read_queue);
@@ -54,6 +54,8 @@
 
 JackCoreMidiOutputPort::~JackCoreMidiOutputPort()
 {
+    // the thread must be gone before the semaphore is closed
+    Stop();
     delete thread;
     sem_close(thread_queue_semaphore);
     sem_unlink(semaphore_name);
@@ -67,10 +69,18 @@
     jack_midi_event_t *event = 0;
     MIDIPacketList *packet_list = (MIDIPacketList *) packet_buffer;
     for (;;) {
+        // Stop() sets this before waking the semaphore
+        if (stopping) {
+            break;
+        }
         MIDIPacket *packet = MIDIPacketListInit(packet_list);
         assert(packet);
         if (! event) {
             event = GetCoreMidiEvent(true);
+            if (! event) {
+                // woken by Stop(), or by a wake-up left over from a previous Stop()
+                continue;
+            }
         }
         jack_midi_data_t *data = event->buffer;
         jack_nframes_t send_frame = event->time;
@@ -227,6 +237,7 @@
 {
     bool result = thread->GetStatus() != JackThread::kIdle;
     if (! result) {
+        stopping = false;
         result = ! thread->StartSync();
         if (! result) {
             jack_error("JackCoreMidiOutputPort::Start - failed to start MIDI "
@@ -241,7 +252,10 @@
 {
     bool result = thread->GetStatus() == JackThread::kIdle;
     if (! result) {
-        result = ! thread->Kill();
+        // Kill() does not make sem_wait() return, wake up the thread and wait for it
+        stopping = true;
+        sem_post(thread_queue_semaphore);
+        result = ! thread->Stop();
         if (! result) {
             jack_error("JackCoreMidiOutputPort::Stop - failed to stop MIDI "
                        "processing thread.");

tests/jack_coremidi_shutdown_test.sh

@@ -0,0 +1,157 @@
+#!/bin/bash
+
+# Test that a jackd server loaded with the CoreMIDI slave driver can be stopped
+# and started again on macOS.
+#
+# Before the fix this test guards, stopping such a server never finished: the CoreMIDI
+# threads did not honour a cancellation request, so jackd stayed blocked while shutting
+# down. A server that did start after a quick restart could also register its MIDI ports
+# twice.
+#
+# The test starts "jackd -X coremidi -d dummy", asks it to stop with SIGTERM, checks that
+# it exits in time, and repeats that several times without any pause in between.
+# No audio device is used.
+#
+# usage:
+#	./jack_coremidi_shutdown_test.sh [path/to/jackd]
+#
+# environment:
+#	JACK_TEST_CYCLES	number of start/stop cycles (default: 5)
+#	JACK_TEST_TIMEOUT	seconds to wait for a start or a stop (default: 10)
+#
+# exit status: 0 on success, 1 on failure, 77 if the test cannot run here (not macOS,
+# jackd not found, or the CoreMIDI driver is not available).
+#
+# Note: jackd keeps a registry of at most 8 server names. A server that had to be killed
+# keeps its entry, so this test always uses the same server name, which is reclaimed
+# the next time it runs.
+
+set -o pipefail
+
+JACKD=${1:-jackd}
+CYCLES=${JACK_TEST_CYCLES:-5}
+TIMEOUT=${JACK_TEST_TIMEOUT:-10}
+SERVER_NAME=jack_coremidi_shutdown_test
+
+LOG=$(mktemp /tmp/jack_coremidi_shutdown_test.XXXXXX)
+JACKD_PID=
+
+#any failed cycle will set this to 1 (error)
+FINAL_RETURN_VALUE=0
+
+function cleanup()
+{
+	if [ -n "$JACKD_PID" ] && kill -0 "$JACKD_PID" 2> /dev/null;
+	then
+		kill -9 "$JACKD_PID" 2> /dev/null
+	fi
+	rm -f "$LOG"
+}
+
+trap cleanup EXIT
+
+function skip()
+{
+	echo "SKIPPED: $1"
+	exit 77
+}
+
+function fail()
+{
+	echo "**	FAILED: $1"
+	FINAL_RETURN_VALUE=1
+}
+
+#wait until the process is gone: $1 = pid, $2 = timeout in seconds
+#return 0 if it exited in time
+function wait_exit()
+{
+	local ticks=$(( $2 * 10 ))
+	while kill -0 "$1" 2> /dev/null;
+	do
+		if [ $ticks -le 0 ];
+		then
+			return 1
+		fi
+		ticks=$(( ticks - 1 ))
+		sleep 0.1
+	done
+	return 0
+}
+
+#kill a server that did not stop on its own. It can take a moment for the kill to
+#be effective if the process is stuck in a kernel call, so try again a few times
+function force_kill()
+{
+	local tries=0
+	while kill -0 "$1" 2> /dev/null && [ $tries -lt 10 ];
+	do
+		kill -9 "$1" 2> /dev/null
+		tries=$(( tries + 1 ))
+		sleep 0.5
+	done
+	#reap the process so that the shell does not report it later
+	{ wait "$1"; } 2> /dev/null
+}
+
+[ "$(uname)" = "Darwin" ] || skip "this test is for macOS"
+command -v "$JACKD" > /dev/null 2>&1 || skip "jackd not found: $JACKD"
+
+for cycle in $(seq 1 "$CYCLES");
+do
+	echo "cycle $cycle/$CYCLES"
+	: > "$LOG"
+	"$JACKD" -n "$SERVER_NAME" -X coremidi -d dummy -r 48000 -p 256 > "$LOG" 2>&1 < /dev/null &
+	JACKD_PID=$!
+
+	#wait for the CoreMIDI driver to be fully started
+	started=0
+	ticks=$(( TIMEOUT * 10 ))
+	while [ $ticks -gt 0 ];
+	do
+		if grep -q "JackCoreMidiDriver::Start - Driver started" "$LOG";
+		then
+			started=1
+			break
+		fi
+		if ! kill -0 "$JACKD_PID" 2> /dev/null;
+		then
+			break
+		fi
+		ticks=$(( ticks - 1 ))
+		sleep 0.1
+	done
+
+	if [ $started -ne 1 ];
+	then
+		if grep -q -E "Driver \"coremidi\" cannot be loaded|Could not find|No such file" "$LOG" && [ "$cycle" -eq 1 ];
+		then
+			force_kill "$JACKD_PID"
+			skip "the CoreMIDI driver is not available"
+		fi
+		fail "the server did not start (cycle $cycle)"
+		sed 's/^/	| /' "$LOG" | tail -n 5
+		force_kill "$JACKD_PID"
+		JACKD_PID=
+		continue
+	fi
+
+	kill -TERM "$JACKD_PID"
+	if wait_exit "$JACKD_PID" "$TIMEOUT";
+	then
+		echo "	OK: stopped"
+	else
+		fail "the server did not stop within $TIMEOUT s (cycle $cycle)"
+		force_kill "$JACKD_PID"
+	fi
+	JACKD_PID=
+done
+
+if [ $FINAL_RETURN_VALUE -eq 0 ];
+then
+	echo "PASSED"
+else
+	echo "FAILED"
+fi
+
+exit $FINAL_RETURN_VALUE