Fix MacOSX deadlock by locking mutex in drawRect instead of holding across threads

.gitignore

@@ -40,4 +40,6 @@ gui-js/documentation/*
 gui-js/dist/*
 gui-js/.nx/*
 gui-js/.angular/*
-doc/minsky/*
+doc/minsky/*
+# CodeQL temporary files
+_codeql_detected_source_root

RESTService/addon.cc

@@ -186,7 +186,10 @@ namespace minsky
   {
     // ensure access to only one global Minsky object at a time,
     // particular needed for jest tests, which run in parallel
-    mutex minskyCmdMutex; 
+    // Use recursive_mutex to allow the same thread to acquire it multiple times
+    // (e.g., JS thread may hold it in doCommand() and then macOSXDrawNativeWindows()
+    // may also be scheduled on the same thread)
+    recursive_mutex minskyCmdMutex; 
 
     struct AddOnMinsky: public RESTMinsky
     {
@@ -235,7 +238,7 @@ namespace minsky
 
       string doCommand(const string& command, const json_pack_t& arguments)
       {
-        const lock_guard<mutex> lock(minskyCmdMutex);
+        const lock_guard<recursive_mutex> lock(minskyCmdMutex);
         const Timer timer(timers[command]);
 
         // if reset requested, postpone it
@@ -250,7 +253,7 @@ namespace minsky
 
       void drawNativeWindows()
       {
-        const lock_guard<mutex> lock(minskyCmdMutex);
+        const lock_guard<recursive_mutex> lock(minskyCmdMutex);
         const Timer timer(timers["draw"]);
         for (auto i: nativeWindowsToRedraw)
           try
@@ -276,19 +279,28 @@ namespace minsky
 
       void macOSXDrawNativeWindows()
       {
-        // share the lock with all window redraw routines - when all windows redrawn, lock is released
-        auto lock=make_shared<lock_guard<mutex>>(minskyCmdMutex);
+        // Don't hold the lock here - pass mutex reference to windows so drawRect can lock when called
         const Timer timer(timers["draw"]);
-        for (auto i: nativeWindowsToRedraw)
-          macOSXRedraw(*i,lock);
+        decltype(nativeWindowsToRedraw) windows;
+        {
+          const lock_guard<recursive_mutex> lock(minskyCmdMutex);
+          windows.swap(nativeWindowsToRedraw);
+        }
+        for (auto i: windows)
+          macOSXRedraw(*i,minskyCmdMutex);
         nativeWindowsToRedraw.clear();
         drawLaunched=false;
        }
 
       void macOSXLaunchDrawNativeWindows()
       {
         drawLaunched=true;
-        tsDrawNativeWindows_.BlockingCall(this);
+        // Use NonBlockingCall instead of BlockingCall to prevent deadlock.
+        // BlockingCall would block the Minsky thread waiting for JS thread to complete,
+        // but JS thread needs to acquire minskyCmdMutex which could be held, causing deadlock.
+        // NonBlockingCall allows Minsky thread to continue, and JS thread will acquire
+        // the mutex when it's ready to run macOSXDrawNativeWindows().
+        tsDrawNativeWindows_.NonBlockingCall(this);
       }
 
       void run()
@@ -313,7 +325,7 @@ namespace minsky
                 if (reset_flag() && resetDuration<resetPostponedThreshold && resetAt<std::chrono::system_clock::now())
                   try
                     {
-                      const lock_guard<mutex> lock(minskyCmdMutex);
+                      const lock_guard<recursive_mutex> lock(minskyCmdMutex);
                       if (reset_flag()) // check again, in case another thread got there first
                         {
                           const Timer timer(timers["minsky.reset"]);

doc/Ravel.tex

@@ -1,4 +1,4 @@
-%% LyX 2.3.7 created this file.  For more info, see http://www.lyx.org/.
+ %% LyX 2.3.7 created this file.  For more info, see http://www.lyx.org/.
 %% Do not edit unless you really know what you are doing.
 \documentclass{book}
 \usepackage[latin9]{inputenc}

model/getContext.cc

@@ -26,6 +26,7 @@
 #include <cairo/cairo-quartz.h>
 #include <iostream>
 #include <exception>
+#include <mutex>
 #include "minsky_epilogue.h"
 
 using namespace std;
@@ -100,13 +101,20 @@ namespace minsky
   void NSContext::requestRedraw()
   {
    [impl->cairoView setNeedsDisplay: true];
+   // Force the view to display immediately instead of waiting for the next event loop
+   [impl->cairoView displayIfNeeded];
   }
 
 }
 
 @implementation CairoView
 -(void) drawRect: (NSRect)rect
 {
+  // Lock the mutex when actually drawing, not before
+  std::optional<std::lock_guard<std::recursive_mutex>> lock;
+  if (winfo->cmdMutex)
+    lock.emplace(*winfo->cmdMutex);
+
   auto context = [[NSGraphicsContext currentContext] CGContext];
   auto frame=[self frame];
   CGContextTranslateCTM(context,winfo->offsetLeft,winfo->childHeight+(winfo->hasScrollBars?20:0)); 
@@ -116,7 +124,7 @@ namespace minsky
     cairo_surface_set_device_offset(winfo->bufferSurface->surface(), 0, 20);
   winfo->draw();
   winfo->bufferSurface.reset();
-  winfo->lock.reset(); // unlock any mutex attached to this window
+  // lock will be automatically released when it goes out of scope
 }
 - (NSView *) hitTest: (NSPoint) aPoint
 {

model/renderNativeWindow.cc

@@ -93,11 +93,11 @@ namespace minsky
     winInfoPtr.reset();
   }
 
-void macOSXRedraw(RenderNativeWindow& window,const std::shared_ptr<std::lock_guard<std::mutex>>& lock)
+void macOSXRedraw(RenderNativeWindow& window,std::recursive_mutex& cmdMutex)
   {
 #ifdef MAC_OSX_TK
     if (!window.winInfoPtr.get()) return;
-    window.winInfoPtr->lock=lock;
+    window.winInfoPtr->cmdMutex=&cmdMutex;
     window.winInfoPtr->requestRedraw();
 #endif
   }

model/renderNativeWindow.h

@@ -25,13 +25,15 @@
 #include <cairoSurfaceImage.h>
 #include <plot.h>
 
-#ifndef CLASSDESC_TYPENAME___std__lock_guard__mutex___
-#define CLASSDESC_TYPENAME___std__lock_guard__mutex___
+#include <mutex>
+
+#ifndef CLASSDESC_TYPENAME___std__lock_guard__recursive_mutex___
+#define CLASSDESC_TYPENAME___std__lock_guard__recursive_mutex___
 namespace classdesc
 {
-  template <> struct tn<std::lock_guard<std::mutex>>
+  template <> struct tn<std::lock_guard<std::recursive_mutex>>
   {
-    static string name() {return "std::lock_guard<std::mutex>";}
+    static string name() {return "std::lock_guard<std::recursive_mutex>";}
   };
 }
 #endif
@@ -65,7 +67,7 @@ namespace minsky
     void draw();
     void requestRedraw();
     // implemented as a free function to avoid Classdesc exposing this to Typescript
-    friend void macOSXRedraw(RenderNativeWindow&,const std::shared_ptr<std::lock_guard<std::mutex>>&);
+    friend void macOSXRedraw(RenderNativeWindow&,std::recursive_mutex&);
     // do not clobber winInfoPtr on load of model
     RenderNativeWindow& operator=(const RenderNativeWindow& x) {ecolab::CairoSurface::operator=(x); return *this;}
     RenderNativeWindow()=default;

model/windowInformation.h

@@ -50,8 +50,8 @@ namespace minsky
     HANDLE hOld;    // 
 #elif defined(MAC_OSX_TK)
     NSContext nsContext;
-    std::shared_ptr<std::lock_guard<std::mutex>> lock;
-    Winfo(NSContext&& nsContext): nsContext(std::move(nsContext)) {}
+    std::recursive_mutex* cmdMutex; // pointer to minskyCmdMutex for locking during draw
+    Winfo(NSContext&& nsContext): nsContext(std::move(nsContext)), cmdMutex(nullptr) {}
 #elif defined(USE_X11)
     Window parentWindowId;
     Window childWindowId, bufferWindowId;