From a6d0b3f902112ad2ec4dfb87ab73fcef1a8d9090 Mon Sep 17 00:00:00 2001 From: Leland Lucius Date: Thu, 16 Apr 2020 22:14:45 -0500 Subject: [PATCH] Bug 2360 - Scripting: "Message:" command may crash when using Nyquist with Python --- src/ModuleManager.cpp | 5 +- src/commands/Command.cpp | 2 +- src/commands/CommandBuilder.cpp | 24 ++-- src/commands/CommandBuilder.h | 5 +- src/commands/CommandTargets.cpp | 6 - src/commands/CommandTargets.h | 32 +++--- src/commands/ScriptCommandRelay.cpp | 168 ++++++++-------------------- src/commands/ScriptCommandRelay.h | 29 +---- src/effects/nyquist/Nyquist.cpp | 4 +- 9 files changed, 89 insertions(+), 186 deletions(-) diff --git a/src/ModuleManager.cpp b/src/ModuleManager.cpp index fea4f5750..546420cb4 100755 --- a/src/ModuleManager.cpp +++ b/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 // 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::SetRegScriptServerFunc(scriptFn); - NonGuiThread::StartChild(&ScriptCommandRelay::Run); + ScriptCommandRelay::StartScriptServer(scriptFn); } } diff --git a/src/commands/Command.cpp b/src/commands/Command.cpp index d41f216ba..c6fb5c30c 100644 --- a/src/commands/Command.cpp +++ b/src/commands/Command.cpp @@ -165,7 +165,7 @@ bool ApplyAndSendResponse::Apply() { response += wxT("Failed!"); } - mCtx->Status(response); + mCtx->Status(response, true); return result; } diff --git a/src/commands/CommandBuilder.cpp b/src/commands/CommandBuilder.cpp index 101284492..5477a1b8b 100644 --- a/src/commands/CommandBuilder.cpp +++ b/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(std::make_unique(), - scriptOutput, - scriptOutput); + mResponse, + 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 ':'"); - ResponseQueueTarget::sResponseQueue().AddResponse( - Response{wxT("\n")}); - mValid = false; + Failure(wxT("Syntax error!\nCommand is missing ':'")); return; } diff --git a/src/commands/CommandBuilder.h b/src/commands/CommandBuilder.h index c9c9310d8..48949209c 100644 --- a/src/commands/CommandBuilder.h +++ b/src/commands/CommandBuilder.h @@ -19,6 +19,8 @@ #include "../MemoryX.h" class AudacityProject; +class ResponseTarget; +using ResponseTargetPointer = std::shared_ptr; class OldStyleCommand; using OldStyleCommandPointer = std::shared_ptr; 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__ */ diff --git a/src/commands/CommandTargets.cpp b/src/commands/CommandTargets.cpp index c1e2578d5..145419e93 100644 --- a/src/commands/CommandTargets.cpp +++ b/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; -} diff --git a/src/commands/CommandTargets.h b/src/commands/CommandTargets.h index 03365c170..95474c0ab 100644 --- a/src/commands/CommandTargets.h +++ b/src/commands/CommandTargets.h @@ -57,7 +57,7 @@ and sends it to that message target. #include "../MemoryX.h" #include -#include "../commands/ResponseQueue.h" +#include 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) -class ResponseQueueTarget final : public CommandMessageTarget +/// Constructs a response (to be sent back to a script) +class ResponseTarget final : public CommandMessageTarget { private: + wxSemaphore mSemaphore; wxString mBuffer; public: - static ResponseQueue &sResponseQueue(); - - ResponseQueueTarget() - : mBuffer( wxEmptyString ) + ResponseTarget() + : mBuffer(wxEmptyString), + mSemaphore(0, 1) { // 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); -#endif + } + virtual void Flush() override + { + mSemaphore.Post(); + } + wxString GetResponse() + { + mSemaphore.Wait(); + return mBuffer; } }; diff --git a/src/commands/ScriptCommandRelay.cpp b/src/commands/ScriptCommandRelay.cpp index 23255e595..4fbe39729 100644 --- a/src/commands/ScriptCommandRelay.cpp +++ b/src/commands/ScriptCommandRelay.cpp @@ -26,150 +26,80 @@ code out of ModuleManager. #include "AppCommandEvent.h" #include "../Project.h" #include -#include #include +#include -// Declare static class members -CommandHandler *ScriptCommandRelay::sCmdHandler; -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) +/// This is the function which actually obeys one command. +static int ExecCommand(wxString *pIn, wxString *pOut, bool fromMain) { { 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 - // expected to be reached from within the XLisp runtime - wxTheApp->SafelyProcessEvent( ev ); - - *pOut = wxEmptyString; + if (fromMain) + { + // Use SafelyProcessEvent, which stops exceptions, because this is + // expected to be reached from within the XLisp runtime + 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. - // The last response is signalled by an empty line. - // - // 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(); + // Wait for and retrieve the response + *pOut = builder.GetResponse(); } - #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 ){ - wxString Str1( pIn ); - wxString Str2; - ExecCommand2( &Str1, &Str2 ); - - // wxString provides a const char * - //const char * pStr = static_cast(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() +void * ExecForLisp( char * pIn ) { - return ResponseQueueTarget::sResponseQueue().WaitAndGetResponse(); + wxString Str1(pIn); + wxString Str2; + + ExecFromMain(&Str1, &Str2); + + return nyq_reformat_aud_do_response(Str2); } diff --git a/src/commands/ScriptCommandRelay.h b/src/commands/ScriptCommandRelay.h index cb9223709..3bb330276 100644 --- a/src/commands/ScriptCommandRelay.h +++ b/src/commands/ScriptCommandRelay.h @@ -20,36 +20,15 @@ #include "../MemoryX.h" -class wxWindow; -class CommandHandler; -class Response; -class OldStyleCommand; -using OldStyleCommandPointer = std::shared_ptr; class wxString; -typedef int (*tpExecScriptServerFunc)( wxString * pIn, wxString * pOut); -typedef int (*tpRegScriptServerFunc)(tpExecScriptServerFunc pFn); - -extern "C" { - AUDACITY_DLL_API int ExecCommand(wxString *pIn, wxString *pOut); -} // End 'extern C' +typedef int(*tpExecScriptServerFunc)(wxString * pIn, wxString * pOut); +typedef int(*tpRegScriptServerFunc)(tpExecScriptServerFunc pFn); class ScriptCommandRelay { - private: - // N.B. Static class members also have to be declared in the .cpp file - 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(); +public: + static void StartScriptServer(tpRegScriptServerFunc scriptFn); }; #endif /* End of include guard: __SCRIPT_COMMAND_RELAY__ */ diff --git a/src/effects/nyquist/Nyquist.cpp b/src/effects/nyquist/Nyquist.cpp index 6754fad31..e9ded20e2 100644 --- a/src/effects/nyquist/Nyquist.cpp +++ b/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;