From 5a8561fc558ea72a5350e7aed98c04443af2874e Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Sun, 6 Jan 2008 17:01:05 +0000 Subject: [PATCH] fix wxExecute thread shutdown and free wxExecuteData even if the associated process is still running (bug 1863908); fix memory leaks when the asynchronously launched processes are still running in the sample too git-svn-id: https://svn.wxwidgets.org/svn/wx/wxWidgets/trunk@51040 c3d73ce0-8a6f-49c7-b76d-6d57e0e08775 --- samples/exec/exec.cpp | 107 ++++++++++++++++++++++++++++-------------- src/msw/utilsexc.cpp | 97 +++++++++++++++++++++++++++++++++----- 2 files changed, 157 insertions(+), 47 deletions(-) diff --git a/samples/exec/exec.cpp b/samples/exec/exec.cpp index 19d2dba153..64d1aae2b5 100644 --- a/samples/exec/exec.cpp +++ b/samples/exec/exec.cpp @@ -81,14 +81,18 @@ public: // Define an array of process pointers used by MyFrame class MyPipedProcess; -WX_DEFINE_ARRAY_PTR(MyPipedProcess *, MyProcessesArray); +WX_DEFINE_ARRAY_PTR(MyPipedProcess *, MyPipedProcessesArray); + +class MyProcess; +WX_DEFINE_ARRAY_PTR(MyProcess *, MyProcessesArray); // Define a new frame type: this is going to be our main frame class MyFrame : public wxFrame { public: - // ctor(s) + // ctor and dtor MyFrame(const wxString& title, const wxPoint& pos, const wxSize& size); + virtual ~MyFrame(); // event handlers (these functions should _not_ be virtual) void OnQuit(wxCommandEvent& event); @@ -122,6 +126,9 @@ public: void OnProcessTerminated(MyPipedProcess *process); wxListBox *GetLogListBox() const { return m_lbox; } + // for MyProcess + void OnAsyncTermination(MyProcess *process); + private: void ShowOutput(const wxString& cmd, const wxArrayString& output, @@ -129,30 +136,11 @@ private: void DoAsyncExec(const wxString& cmd); - void AddAsyncProcess(MyPipedProcess *process) - { - if ( m_running.IsEmpty() ) - { - // we want to start getting the timer events to ensure that a - // steady stream of idle events comes in -- otherwise we - // wouldn't be able to poll the child process input - m_timerIdleWakeUp.Start(100); - } - //else: the timer is already running + void AddAsyncProcess(MyProcess *process) { m_allAsync.push_back(process); } - m_running.Add(process); - } - - void RemoveAsyncProcess(MyPipedProcess *process) - { - m_running.Remove(process); + void AddPipedProcess(MyPipedProcess *process); + void RemovePipedProcess(MyPipedProcess *process); - if ( m_running.IsEmpty() ) - { - // we don't need to get idle events all the time any more - m_timerIdleWakeUp.Stop(); - } - } // the PID of the last process we launched asynchronously long m_pidLast; @@ -174,7 +162,11 @@ private: wxListBox *m_lbox; - MyProcessesArray m_running; + // array of running processes with redirected IO + MyPipedProcessesArray m_running; + + // array of all asynchrously running processes + MyProcessesArray m_allAsync; // the idle event wake up timer wxTimer m_timerIdleWakeUp; @@ -501,6 +493,17 @@ MyFrame::MyFrame(const wxString& title, const wxPoint& pos, const wxSize& size) #endif // wxUSE_STATUSBAR } +MyFrame::~MyFrame() +{ + // any processes left until now must be deleted manually: normally this is + // done when the associated process terminates but it must be still running + // if this didn't happen until now + for ( size_t n = 0; n < m_allAsync.size(); n++ ) + { + delete m_allAsync[n]; + } +} + // ---------------------------------------------------------------------------- // event handlers: file and help menu // ---------------------------------------------------------------------------- @@ -637,20 +640,23 @@ void MyFrame::OnKill(wxCommandEvent& WXUNUSED(event)) void MyFrame::DoAsyncExec(const wxString& cmd) { - wxProcess *process = new MyProcess(this, cmd); + MyProcess * const process = new MyProcess(this, cmd); m_pidLast = wxExecute(cmd, wxEXEC_ASYNC, process); if ( !m_pidLast ) { - wxLogError( _T("Execution of '%s' failed."), cmd.c_str() ); + wxLogError(_T("Execution of '%s' failed."), cmd.c_str()); delete process; } else { - wxLogStatus( _T("Process %ld (%s) launched."), - m_pidLast, cmd.c_str() ); + wxLogStatus(_T("Process %ld (%s) launched."), m_pidLast, cmd.c_str()); m_cmdLast = cmd; + + // the parent frame keeps track of all async processes as it needs to + // free them if we exit before the child process terminates + AddAsyncProcess(process); } } @@ -772,7 +778,7 @@ void MyFrame::OnExecWithRedirect(wxCommandEvent& WXUNUSED(event)) } else { - AddAsyncProcess(process); + AddPipedProcess(process); } } @@ -801,9 +807,9 @@ void MyFrame::OnExecWithPipe(wxCommandEvent& WXUNUSED(event)) long pid = wxExecute(cmd, wxEXEC_ASYNC, process); if ( pid ) { - wxLogStatus( _T("Process %ld (%s) launched."), pid, cmd.c_str() ); + wxLogStatus(_T("Process %ld (%s) launched."), pid, cmd.c_str()); - AddAsyncProcess(process); + AddPipedProcess(process); } else { @@ -1019,9 +1025,41 @@ void MyFrame::OnTimer(wxTimerEvent& WXUNUSED(event)) void MyFrame::OnProcessTerminated(MyPipedProcess *process) { - RemoveAsyncProcess(process); + RemovePipedProcess(process); +} + +void MyFrame::OnAsyncTermination(MyProcess *process) +{ + m_allAsync.Remove(process); + + delete process; +} + +void MyFrame::AddPipedProcess(MyPipedProcess *process) +{ + if ( m_running.IsEmpty() ) + { + // we want to start getting the timer events to ensure that a + // steady stream of idle events comes in -- otherwise we + // wouldn't be able to poll the child process input + m_timerIdleWakeUp.Start(100); + } + //else: the timer is already running + + m_running.Add(process); + m_allAsync.Add(process); } +void MyFrame::RemovePipedProcess(MyPipedProcess *process) +{ + m_running.Remove(process); + + if ( m_running.IsEmpty() ) + { + // we don't need to get idle events all the time any more + m_timerIdleWakeUp.Stop(); + } +} void MyFrame::ShowOutput(const wxString& cmd, const wxArrayString& output, @@ -1052,8 +1090,7 @@ void MyProcess::OnTerminate(int pid, int status) wxLogStatus(m_parent, _T("Process %u ('%s') terminated with exit code %d."), pid, m_cmd.c_str(), status); - // we're not needed any more - delete this; + m_parent->OnAsyncTermination(this); } // ---------------------------------------------------------------------------- diff --git a/src/msw/utilsexc.cpp b/src/msw/utilsexc.cpp index fa2c29b351..2194678500 100644 --- a/src/msw/utilsexc.cpp +++ b/src/msw/utilsexc.cpp @@ -38,6 +38,7 @@ #include "wx/process.h" #include "wx/thread.h" #include "wx/apptrait.h" +#include "wx/vector.h" #include "wx/msw/private.h" @@ -101,6 +102,13 @@ wxCreateHiddenWindow(LPCTSTR *pclassname, LPCTSTR classname, WNDPROC wndproc); static const wxChar *wxMSWEXEC_WNDCLASSNAME = wxT("_wxExecute_Internal_Class"); static const wxChar *gs_classForHiddenWindow = NULL; +// event used to wake up threads waiting in wxExecuteThread +static HANDLE gs_heventShutdown = NULL; + +// handles of all threads monitoring the execution of asynchronously running +// processes +static wxVector gs_asyncThreads; + // ---------------------------------------------------------------------------- // private types // ---------------------------------------------------------------------------- @@ -131,6 +139,43 @@ public: virtual bool OnInit() { return true; } virtual void OnExit() { + if ( gs_heventShutdown ) + { + // stop any threads waiting for the termination of asynchronously + // running processes + if ( !::SetEvent(gs_heventShutdown) ) + { + wxLogDebug(_T("Failed to set shutdown event in wxExecuteModule")); + } + + ::CloseHandle(gs_heventShutdown); + gs_heventShutdown = NULL; + + // now wait until they terminate + if ( !gs_asyncThreads.empty() ) + { + const size_t numThreads = gs_asyncThreads.size(); + + if ( ::WaitForMultipleObjects + ( + numThreads, + &gs_asyncThreads[0], + TRUE, // wait for all of them to become signalled + 3000 // long but finite value + ) == WAIT_TIMEOUT ) + { + wxLogDebug(_T("Failed to stop all wxExecute monitor threads")); + } + + for ( size_t n = 0; n < numThreads; n++ ) + { + ::CloseHandle(gs_asyncThreads[n]); + } + + gs_asyncThreads.clear(); + } + } + if ( *gs_classForHiddenWindow ) { if ( !::UnregisterClass(wxMSWEXEC_WNDCLASSNAME, wxGetInstance()) ) @@ -146,6 +191,8 @@ private: DECLARE_DYNAMIC_CLASS(wxExecuteModule) }; +IMPLEMENT_DYNAMIC_CLASS(wxExecuteModule, wxModule) + #if wxUSE_STREAMS && !defined(__WXWINCE__) // ---------------------------------------------------------------------------- @@ -281,22 +328,48 @@ static DWORD __stdcall wxExecuteThread(void *arg) { wxExecuteData * const data = (wxExecuteData *)arg; - if ( ::WaitForSingleObject(data->hProcess, INFINITE) != WAIT_OBJECT_0 ) + // create the shutdown event if we're the first thread starting to wait + if ( !gs_heventShutdown ) { - wxLogDebug(_T("Waiting for the process termination failed!")); + // create a manual initially non-signalled event object + gs_heventShutdown = ::CreateEvent(NULL, TRUE, FALSE, NULL); + if ( !gs_heventShutdown ) + { + wxLogDebug(_T("CreateEvent() in wxExecuteThread failed")); + } } - // get the exit code - if ( !::GetExitCodeProcess(data->hProcess, &data->dwExitCode) ) + HANDLE handles[2] = { data->hProcess, gs_heventShutdown }; + switch ( ::WaitForMultipleObjects(2, handles, FALSE, INFINITE) ) { - wxLogLastError(wxT("GetExitCodeProcess")); - } + case WAIT_OBJECT_0: + // process terminated, get its exit code + if ( !::GetExitCodeProcess(data->hProcess, &data->dwExitCode) ) + { + wxLogLastError(wxT("GetExitCodeProcess")); + } + + wxASSERT_MSG( data->dwExitCode != STILL_ACTIVE, + wxT("process should have terminated") ); - wxASSERT_MSG( data->dwExitCode != STILL_ACTIVE, - wxT("process should have terminated") ); + // send a message indicating process termination to the window + ::SendMessage(data->hWnd, wxWM_PROC_TERMINATED, 0, (LPARAM)data); + break; + + case WAIT_OBJECT_0 + 1: + // we're shutting down but the process is still running -- leave it + // run but clean up the associated data + if ( !data->state ) + { + delete data; + } + //else: exiting while synchronously executing process is still + // running? this shouldn't happen... + break; - // send a message indicating process termination to the window - ::SendMessage(data->hWnd, wxWM_PROC_TERMINATED, 0, (LPARAM)data); + default: + wxLogDebug(_T("Waiting for the process termination failed!")); + } return 0; } @@ -321,7 +394,7 @@ LRESULT APIENTRY _EXPORT wxExecuteWindowCbk(HWND hWnd, UINT message, { // we're executing synchronously, tell the waiting thread // that the process finished - data->state = 0; + data->state = false; } else { @@ -850,7 +923,7 @@ long wxExecute(const wxString& cmd, int flags, wxProcess *handler) return pi.dwProcessId; } - ::CloseHandle(hThread); + gs_asyncThreads.push_back(hThread); #if wxUSE_IPC && !defined(__WXWINCE__) // second part of DDE hack: now establish the DDE conversation with the -- 2.47.2