Redo some thread handing in the clock to avoid deadlocks on windows and macos

Author: blurpy

src/core/Clock.cpp

@@ -58,7 +58,6 @@ void Core::Clock::stop() {
 void Core::Clock::halt() {
     halted = true;
     stop();
-    clockThread.detach(); // To allow creating a new thread without crashing if you don't join() first
 }
 
 void Core::Clock::join() {
@@ -67,6 +66,12 @@ void Core::Clock::join() {
     }
 }
 
+void Core::Clock::detach() {
+    if (clockThread.joinable()) {
+        clockThread.detach();
+    }
+}
+
 void Core::Clock::singleStep() {
     std::cout << "Clock: single stepping clock" << std::endl;
 

src/core/Clock.h

@@ -28,8 +28,9 @@ namespace Core {
         ~Clock();
 
         /**
-         * Start the clock asynchronously.
-         * Will continue until stopped programmatically or through the HLT instruction.
+         * Start the clock asynchronously. Will continue until stop() or halt().
+         * Note: you need to either call join() or detach() right afterwards otherwise you may experience
+         * crashes or deadlocks.
          */
         void start();
 
@@ -42,6 +43,9 @@ namespace Core {
         /** Wait for an asynchronous clock while it's running. */
         void join();
 
+        /** Ignore an asynchronous clock while it's running. */
+        void detach();
+
         /** Run one clock cycle synchronously and then stop. */
         void singleStep();
 

src/core/Emulator.cpp

@@ -67,12 +67,22 @@ void Core::Emulator::reload() {
     initializeProgram();
 }
 
-void Core::Emulator::run() {
+void Core::Emulator::startAsynchronous() {
     if (Utils::debugL1()) {
-        std::cout << "Emulator: run start" << std::endl;
+        std::cout << "Emulator: start asynchronous" << std::endl;
     }
 
     clock->start();
+    clock->detach();
+}
+
+void Core::Emulator::startSynchronous() {
+    if (Utils::debugL1()) {
+        std::cout << "Emulator: start synchronous" << std::endl;
+    }
+
+    clock->start();
+    clock->join();
 }
 
 void Core::Emulator::initializeProgram() {
@@ -97,10 +107,6 @@ bool Core::Emulator::isRunning() {
     return clock->isRunning();
 }
 
-void Core::Emulator::waitUntilFinished() {
-    clock->join();
-}
-
 void Core::Emulator::singleStep() {
     clock->singleStep();
 }

src/core/Emulator.h

@@ -34,10 +34,10 @@ namespace Core {
         void reload();
 
         /** Start running the loaded program. Asynchronous. */
-        void run();
+        void startAsynchronous();
 
-        /** Wait for the program to finish, or for the emulator to finish stopping. */
-        void waitUntilFinished();
+        /** Start running the loaded program. Synchronous. */
+        void startSynchronous();
 
         /** Whether the emulator is currently running a program. */
         bool isRunning();

src/ui/Keyboard.cpp

@@ -23,9 +23,8 @@ void UI::Keyboard::keyUp(const SDL_Keycode keycode) {
     if (keycode == SDLK_s) {
         if (emulator->isRunning()) {
             emulator->stop();
-            emulator->waitUntilFinished();
         } else {
-            emulator->run();
+            emulator->startAsynchronous();
         }
     }
 

src/ui/UserInterface.cpp

@@ -99,7 +99,6 @@ void UI::UserInterface::mainLoop() {
     }
 
     emulator->stop();
-    emulator->waitUntilFinished();
 }
 
 void UI::UserInterface::drawLeftColumn() {

test/core/ClockTest.cpp

@@ -241,6 +241,20 @@ TEST_SUITE("ClockTest") {
             fakeit::VerifyNoOtherInvocations(listenerMock);
         }
 
+        SUBCASE("detach() should allow restarting the clock without join()") {
+            clock.setFrequency(5);
+            clock.start();
+
+            clock.detach();
+            std::this_thread::sleep_for(std::chrono::milliseconds(100));
+
+            clock.stop();
+            std::this_thread::sleep_for(std::chrono::milliseconds(100));
+            CHECK_FALSE(clock.isRunning());
+
+            clock.singleStep();
+        }
+
         SUBCASE("setFrequency() should notify observer") {
             fakeit::Mock<ClockObserver> observerMock;
             auto observerPtr = std::shared_ptr<ClockObserver>(&observerMock(), [](...) {});

test/core/EmulatorIntegrationTest.cpp

@@ -27,55 +27,50 @@ TEST_SUITE("EmulatorIntegrationTest") {
                               "Emulator: no instructions loaded. Aborting");
         }
 
-        SUBCASE("run() should complete nop_test.asm") {
+        SUBCASE("startSynchronous() should complete nop_test.asm") {
             emulator.load("../../programs/nop_test.asm");
 
-            emulator.run();
-            emulator.waitUntilFinished();
+            emulator.startSynchronous();
 
             fakeit::Verify(Method(observerMock, valueUpdated).Using(0)).Once(); // Reset before start
             fakeit::Verify(Method(observerMock, valueUpdated).Using(10)).Once();
             fakeit::VerifyNoOtherInvocations(observerMock);
         }
 
-        SUBCASE("run() should complete add_two_numbers.asm") {
+        SUBCASE("startSynchronous() should complete add_two_numbers.asm") {
             emulator.load("../../programs/add_two_numbers.asm");
 
-            emulator.run();
-            emulator.waitUntilFinished();
+            emulator.startSynchronous();
 
             fakeit::Verify(Method(observerMock, valueUpdated).Using(0)).Once(); // Reset before start
             fakeit::Verify(Method(observerMock, valueUpdated).Using(42)).Once(); // 28+14
             fakeit::VerifyNoOtherInvocations(observerMock);
         }
 
-        SUBCASE("run() should complete subtract_two_numbers.asm") {
+        SUBCASE("startSynchronous() should complete subtract_two_numbers.asm") {
             emulator.load("../../programs/subtract_two_numbers.asm");
 
-            emulator.run();
-            emulator.waitUntilFinished();
+            emulator.startSynchronous();
 
             fakeit::Verify(Method(observerMock, valueUpdated).Using(0)).Once(); // Reset before start
             fakeit::Verify(Method(observerMock, valueUpdated).Using(18)).Once(); // 30-12
             fakeit::VerifyNoOtherInvocations(observerMock);
         }
 
-        SUBCASE("run() should complete multiply_two_numbers.asm") {
+        SUBCASE("startSynchronous() should complete multiply_two_numbers.asm") {
             emulator.load("../../programs/multiply_two_numbers.asm");
 
-            emulator.run();
-            emulator.waitUntilFinished();
+            emulator.startSynchronous();
 
             fakeit::Verify(Method(observerMock, valueUpdated).Using(0)).Once(); // Reset before start
             fakeit::Verify(Method(observerMock, valueUpdated).Using(56)).Once(); // 7*8
             fakeit::VerifyNoOtherInvocations(observerMock);
         }
 
-        SUBCASE("run() should complete count_0_255_stop.asm") {
+        SUBCASE("startSynchronous() should complete count_0_255_stop.asm") {
             emulator.load("../../programs/count_0_255_stop.asm");
 
-            emulator.run();
-            emulator.waitUntilFinished();
+            emulator.startSynchronous();
 
             // Once for reset, and once when counting
             fakeit::Verify(Method(observerMock, valueUpdated).Using(0)).Twice();
@@ -87,11 +82,10 @@ TEST_SUITE("EmulatorIntegrationTest") {
             fakeit::VerifyNoOtherInvocations(observerMock);
         }
 
-        SUBCASE("run() should complete count_255_0_stop.asm") {
+        SUBCASE("startSynchronous() should complete count_255_0_stop.asm") {
             emulator.load("../../programs/count_255_0_stop.asm");
 
-            emulator.run();
-            emulator.waitUntilFinished();
+            emulator.startSynchronous();
 
             // Once for reset, and once when counting
             fakeit::Verify(Method(observerMock, valueUpdated).Using(0)).Twice();