Bug 2360 - Scripting: "Message:" command may crash when using Nyquist with Python

src/ModuleManager.cpp

@@ -32,7 +32,6 @@ i.e. an alternative to the usual interface, for Audacity.
 #include "PluginManager.h"
 
 #include "commands/ScriptCommandRelay.h"
-#include <NonGuiThread.h>  // header from libwidgetextra
 
 #include "audacity/PluginInterface.h"
 
@@ -362,9 +361,7 @@ void ModuleManager::Initialize(CommandHandler &cmdHandler)
    // After loading all the modules, we may have a registered scripting function.
    if(scriptFn)
    {
-      ScriptCommandRelay::SetCommandHandler(cmdHandler);
+      ScriptCommandRelay::StartScriptServer(scriptFn);
-      ScriptCommandRelay::SetRegScriptServerFunc(scriptFn);
-      NonGuiThread::StartChild(&ScriptCommandRelay::Run);
    }
 }
 

src/commands/Command.cpp

@@ -165,7 +165,7 @@ bool ApplyAndSendResponse::Apply()
    {
       response += wxT("Failed!");
    }
-   mCtx->Status(response);
+   mCtx->Status(response, true);
    return result;
 }
 

src/commands/CommandBuilder.cpp

@@ -53,11 +53,6 @@ bool CommandBuilder::WasValid()
    return mValid;
 }
 
-const wxString &CommandBuilder::GetErrorMessage()
-{
-   return mError;
-}
-
 OldStyleCommandPointer CommandBuilder::GetCommand()
 {
    wxASSERT(mValid);
@@ -67,6 +62,14 @@ OldStyleCommandPointer CommandBuilder::GetCommand()
    return result;
 }
 
+wxString CommandBuilder::GetResponse()
+{
+   if (!mValid && !mError.empty()) {
+      return mError + wxT("\n");
+   }
+   return mResponse->GetResponse() + wxT("\n");
+}
+
 void CommandBuilder::Failure(const wxString &msg)
 {
    mError = msg;
@@ -85,11 +88,11 @@ void CommandBuilder::BuildCommand(AudacityProject *project,
 {
    // Stage 1: create a Command object of the right type
 
-   auto scriptOutput = std::make_shared< ResponseQueueTarget >();
+   mResponse = std::make_shared< ResponseTarget >();
    auto output
       = std::make_unique<CommandOutputTargets>(std::make_unique<NullProgressTarget>(),
-                                scriptOutput,
+                                mResponse,
-                                scriptOutput);
+                                mResponse);
 
 #ifdef OLD_BATCH_SYSTEM
    OldStyleCommandType *factory = CommandDirectory::Get()->LookUp(cmdName);
@@ -194,10 +197,7 @@ void CommandBuilder::BuildCommand(
    cmdString.Trim(true); cmdString.Trim(false);
    int splitAt = cmdString.Find(wxT(':'));
    if (splitAt < 0 && cmdString.Find(wxT(' ')) >= 0) {
-      mError = wxT("Command is missing ':'");
+      Failure(wxT("Syntax error!\nCommand is missing ':'"));
-      ResponseQueueTarget::sResponseQueue().AddResponse(
-         Response{wxT("\n")});
-      mValid = false;
       return;
    }
 

src/commands/CommandBuilder.h

@@ -19,6 +19,8 @@
 #include "../MemoryX.h"
 
 class AudacityProject;
+class ResponseTarget;
+using ResponseTargetPointer = std::shared_ptr<ResponseTarget>;
 class OldStyleCommand;
 using OldStyleCommandPointer = std::shared_ptr<OldStyleCommand>;
 class wxString;
@@ -30,6 +32,7 @@ class CommandBuilder
 {
    private:
       bool mValid;
+      ResponseTargetPointer mResponse;
       OldStyleCommandPointer mCommand;
       wxString mError;
 
@@ -45,6 +48,6 @@ class CommandBuilder
       ~CommandBuilder();
       bool WasValid();
       OldStyleCommandPointer GetCommand();
-      const wxString &GetErrorMessage();
+      wxString GetResponse();
 };
 #endif /* End of include guard: __COMMANDBUILDER__ */

src/commands/CommandTargets.cpp

@@ -463,9 +463,3 @@ void StatusBarTarget::Update(const wxString &message)
 {
    mStatus.SetStatusText(message, 0);
 }
-
-ResponseQueue &ResponseQueueTarget::sResponseQueue()
-{
-   static ResponseQueue queue;
-   return queue;
-}

src/commands/CommandTargets.h

@@ -57,7 +57,7 @@ and sends it to that message target.
 
 #include "../MemoryX.h"
 #include <vector>
-#include "../commands/ResponseQueue.h"
+#include <wx/thread.h>
 
 class wxStatusBar;
 
@@ -221,33 +221,35 @@ public:
    void Update(const wxString &message) override;
 };
 
-/// Adds messages to the global response queue (to be sent back to a script)
+/// Constructs a response (to be sent back to a script)
-class ResponseQueueTarget final : public CommandMessageTarget
+class ResponseTarget final : public CommandMessageTarget
 {
 private:
+   wxSemaphore mSemaphore;
    wxString mBuffer;
 public:
-   static ResponseQueue &sResponseQueue();
+   ResponseTarget()
-
+      : mBuffer(wxEmptyString),
-   ResponseQueueTarget()
+        mSemaphore(0, 1)
-      : mBuffer( wxEmptyString )
    { 
       // Cater for handling long responses quickly.
       mBuffer.Alloc(40000);
    }
-   virtual ~ResponseQueueTarget()
+   virtual ~ResponseTarget()
    {
-      if( mBuffer.StartsWith("\n" ) )
-         mBuffer = mBuffer.Mid( 1 );
-      sResponseQueue().AddResponse( mBuffer  );
-      sResponseQueue().AddResponse(wxString(wxT("\n")));
    }
    void Update(const wxString &message) override
    {
       mBuffer += message;
-#if 0
+   }
-         mResponseQueue.AddResponse(message);
+   virtual void Flush() override
-#endif
+   {
+      mSemaphore.Post();
+   }
+   wxString GetResponse()
+   {
+      mSemaphore.Wait();
+      return mBuffer;
    }
 };
 

src/commands/ScriptCommandRelay.cpp

@@ -26,150 +26,80 @@ code out of ModuleManager.
 #include "AppCommandEvent.h"
 #include "../Project.h"
 #include <wx/app.h>
-#include <wx/window.h>
 #include <wx/string.h>
+#include <thread>
 
-// Declare static class members
+/// This is the function which actually obeys one command.
-CommandHandler *ScriptCommandRelay::sCmdHandler;
+static int ExecCommand(wxString *pIn, wxString *pOut, bool fromMain)
-tpRegScriptServerFunc ScriptCommandRelay::sScriptFn;
-
-void ScriptCommandRelay::SetRegScriptServerFunc(tpRegScriptServerFunc scriptFn)
-{
-   sScriptFn = scriptFn;
-}
-
-void ScriptCommandRelay::SetCommandHandler(CommandHandler &ch)
-{
-   sCmdHandler = &ch;
-}
-
-/// Calls the script function, passing it the function for obeying commands
-void ScriptCommandRelay::Run()
-{
-   wxASSERT( sScriptFn != NULL );
-   while( true )
-      sScriptFn(&ExecCommand);
-}
-
-/// Send a command to a project, to be applied in that context.
-void ScriptCommandRelay::PostCommand(
-   wxWindow *pWindow, const OldStyleCommandPointer &cmd)
-{
-   wxASSERT( pWindow );
-   wxASSERT(cmd != NULL);
-   if ( pWindow ) {
-      AppCommandEvent ev;
-      ev.SetCommand(cmd);
-      pWindow->GetEventHandler()->AddPendingEvent(ev);
-   }
-}
-
-/// This is the function which actually obeys one command.  Rather than applying
-/// the command directly, an event containing a reference to the command is sent
-/// to the main (GUI) thread. This is because having more than one thread access
-/// the GUI at a time causes problems with wxwidgets.
-int ExecCommand(wxString *pIn, wxString *pOut)
 {
    {
       CommandBuilder builder(::GetActiveProject(), *pIn);
       if (builder.WasValid())
       {
          OldStyleCommandPointer cmd = builder.GetCommand();
-         ScriptCommandRelay::PostCommand(wxTheApp->GetTopWindow(), cmd);
 
-         *pOut = wxEmptyString;
-      }
-      else
-      {
-         *pOut = wxT("Syntax error!\n");
-         *pOut += builder.GetErrorMessage() + wxT("\n");
-      }
-   }
-
-   // Wait until all responses from the command have been received.
-   // The last response is signalled by an empty line.
-   wxString msg = ScriptCommandRelay::ReceiveResponse().GetMessage();
-   while (msg != wxT("\n"))
-   {
-      //wxLogDebug( "Msg: %s", msg );
-      *pOut += msg + wxT("\n");
-      msg = ScriptCommandRelay::ReceiveResponse().GetMessage();
-   }
-
-   return 0;
-}
-
-/// This is the function which actually obeys one command.  Rather than applying
-/// the command directly, an event containing a reference to the command is sent
-/// to the main (GUI) thread. This is because having more than one thread access
-/// the GUI at a time causes problems with wxwidgets.
-int ExecCommand2(wxString *pIn, wxString *pOut)
-{
-   {
-      CommandBuilder builder(::GetActiveProject(), *pIn);
-      if (builder.WasValid())
-      {
-         OldStyleCommandPointer cmd = builder.GetCommand();
          AppCommandEvent ev;
          ev.SetCommand(cmd);
 
-         // Use SafelyProcessEvent, which stops exceptions, because this is
+         if (fromMain)
-         // expected to be reached from within the XLisp runtime
+         {
-         wxTheApp->SafelyProcessEvent( ev );
+            // Use SafelyProcessEvent, which stops exceptions, because this is
-
+            // expected to be reached from within the XLisp runtime
-         *pOut = wxEmptyString;
+            wxTheApp->SafelyProcessEvent(ev);
+         }
+         else
+         {
+            // Send the event to the main thread
+            wxTheApp->AddPendingEvent(ev);
+         }
       }
-      else
-      {
-         *pOut = wxT("Syntax error!\n");
-         *pOut += builder.GetErrorMessage() + wxT("\n");
-      }
-   }
 
-   // Wait until all responses from the command have been received.
+      // Wait for and retrieve the response
-   // The last response is signalled by an empty line.
+      *pOut = builder.GetResponse();
-   //
-   // LLL: Allow ExecCommand() to process the responses, otherwise
-   //      it will hang waiting for more responses that will not be
-   //      forthcoming.
-   #if 0
-   wxString msg = ScriptCommandRelay::ReceiveResponse().GetMessage();
-   while (msg != wxT("\n"))
-   {
-      //wxLogDebug( "Msg: %s", msg );
-      *pOut += msg + wxT("\n");
-      msg = ScriptCommandRelay::ReceiveResponse().GetMessage();
    }
-   #endif
 
    return 0;
 }
 
+/// Executes a command in the worker (script) thread
+static int ExecFromWorker(wxString *pIn, wxString *pOut)
+{
+   return ExecCommand(pIn, pOut, false);
+}
 
+/// Executes a command on the main (GUI) thread.
+static int ExecFromMain(wxString *pIn, wxString *pOut)
+{
+   return ExecCommand(pIn, pOut, true);
+}
+
+/// Starts the script server
+void ScriptCommandRelay::StartScriptServer(tpRegScriptServerFunc scriptFn)
+{
+   wxASSERT(scriptFn != NULL);
+
+   auto server = [](tpRegScriptServerFunc function)
+   {
+      while (true)
+      {
+         function(ExecFromWorker);
+      }
+   };
+
+   std::thread(server, scriptFn).detach();
+}
 
 // The void * return is actually a Lisp LVAL and will be cast to such as needed.
 extern void * ExecForLisp( char * pIn );
 extern void * nyq_make_opaque_string( int size, unsigned char *src );
 extern void * nyq_reformat_aud_do_response(const wxString & Str);
 
-
+void * ExecForLisp( char * pIn )
-void * ExecForLisp( char * pIn ){
-   wxString Str1( pIn );
-   wxString Str2;
-   ExecCommand2( &Str1, &Str2 );
-
-   // wxString provides a const char *
-   //const char * pStr = static_cast<const char*>(Str2);
-
-   // We'll be passing it as a non-const unsigned char *
-   // That 'unsafe' cast is actually safe.  nyq_make_opaque_string is just copying the string.
-   void * pResult = nyq_reformat_aud_do_response( Str2 );
-   return pResult;
-};
-
-
-/// Gets a response from the queue (may block)
-Response ScriptCommandRelay::ReceiveResponse()
 {
-   return ResponseQueueTarget::sResponseQueue().WaitAndGetResponse();
+   wxString Str1(pIn);
+   wxString Str2;
+
+   ExecFromMain(&Str1, &Str2);
+
+   return nyq_reformat_aud_do_response(Str2);
 }

src/commands/ScriptCommandRelay.h

@@ -20,36 +20,15 @@
 
 #include "../MemoryX.h"
 
-class wxWindow;
-class CommandHandler;
-class Response;
-class OldStyleCommand;
-using OldStyleCommandPointer = std::shared_ptr<OldStyleCommand>;
 class wxString;
 
-typedef int (*tpExecScriptServerFunc)( wxString * pIn, wxString * pOut);
+typedef int(*tpExecScriptServerFunc)(wxString * pIn, wxString * pOut);
-typedef int (*tpRegScriptServerFunc)(tpExecScriptServerFunc pFn);
+typedef int(*tpRegScriptServerFunc)(tpExecScriptServerFunc pFn);
-
-extern "C" {
-      AUDACITY_DLL_API int ExecCommand(wxString *pIn, wxString *pOut);
-} // End 'extern C'
 
 class ScriptCommandRelay
 {
-   private:
+public:
-      // N.B. Static class members also have to be declared in the .cpp file
+   static void StartScriptServer(tpRegScriptServerFunc scriptFn);
-      static CommandHandler *sCmdHandler;
-      static tpRegScriptServerFunc sScriptFn;
-
-   public:
-
-      static void SetRegScriptServerFunc(tpRegScriptServerFunc scriptFn);
-      static void SetCommandHandler(CommandHandler &ch);
-
-      static void Run();
-      static void PostCommand(
-         wxWindow *pWindow, const OldStyleCommandPointer &cmd);
-      static Response ReceiveResponse();
 };
 
 #endif /* End of include guard: __SCRIPT_COMMAND_RELAY__ */

src/effects/nyquist/Nyquist.cpp

@@ -1449,9 +1449,7 @@ bool NyquistEffect::ProcessOne()
       // communicated back to C++
       auto msg = Verbatim( NyquistToWxString(nyx_get_string()) );
       if (!msg.empty()) { // Empty string may be used as a No-Op return value.
-         if (!mExternal) {
+         Effect::MessageBox( msg );
-            Effect::MessageBox( msg );
-         }
       }
       else {
          return true;