From ab4cf883367eb3818fc0e6dea57d39d21f9ec618 Mon Sep 17 00:00:00 2001 From: grisha vanika Date: Sun, 1 Oct 2017 20:14:14 +0300 Subject: [PATCH] Fix Folder as Workspace crash and "queue overflow" issues Fix crash of Folder as Workspace when too many directory changes happen: Remove limit for amount of queued directory changes: use auto-reset Event instead of Semaphore. Fix #6005, fix #5907, fix #3740, close #6005 --- PowerEditor/src/NppIO.cpp | 11 ++-- .../WinControls/FileBrowser/fileBrowser.cpp | 6 +- .../ReadDirectoryChanges.cpp | 12 +--- .../ReadDirectoryChanges.h | 9 +-- .../ReadDirectoryChangesPrivate.cpp | 7 +-- .../ReadDirectoryChanges/ThreadSafeQueue.h | 59 ++++--------------- 6 files changed, 24 insertions(+), 80 deletions(-) diff --git a/PowerEditor/src/NppIO.cpp b/PowerEditor/src/NppIO.cpp index 95d623e8f..8be894b1f 100644 --- a/PowerEditor/src/NppIO.cpp +++ b/PowerEditor/src/NppIO.cpp @@ -39,7 +39,6 @@ using namespace std; - DWORD WINAPI Notepad_plus::monitorFileOnChange(void * params) { MonitorInfo *monitorInfo = static_cast(params); @@ -77,13 +76,11 @@ DWORD WINAPI Notepad_plus::monitorFileOnChange(void * params) case WAIT_OBJECT_0 + 1: // We've received a notification in the queue. { - if (changes.CheckOverflow()) - printStr(L"Queue overflowed."); - else + DWORD dwAction; + CStringW wstrFilename; + // Process all available changes, ignore User actions + while (changes.Pop(dwAction, wstrFilename)) { - DWORD dwAction; - CStringW wstrFilename; - changes.Pop(dwAction, wstrFilename); generic_string fn = wstrFilename.GetString(); // Fix monitoring files which are under root problem diff --git a/PowerEditor/src/WinControls/FileBrowser/fileBrowser.cpp b/PowerEditor/src/WinControls/FileBrowser/fileBrowser.cpp index b376f991e..33e3e043a 100644 --- a/PowerEditor/src/WinControls/FileBrowser/fileBrowser.cpp +++ b/PowerEditor/src/WinControls/FileBrowser/fileBrowser.cpp @@ -1354,11 +1354,9 @@ DWORD WINAPI FolderUpdater::watching(void *params) { DWORD dwAction; CStringW wstrFilename; - if (changes.CheckOverflow()) - printStr(L"Queue overflowed."); - else + // Process all available changes, ignore User actions + while (changes.Pop(dwAction, wstrFilename)) { - changes.Pop(dwAction, wstrFilename); static generic_string oldName; static std::vector file2Change; file2Change.clear(); diff --git a/PowerEditor/src/WinControls/ReadDirectoryChanges/ReadDirectoryChanges.cpp b/PowerEditor/src/WinControls/ReadDirectoryChanges/ReadDirectoryChanges.cpp index 8467f6f95..0351630d4 100644 --- a/PowerEditor/src/WinControls/ReadDirectoryChanges/ReadDirectoryChanges.cpp +++ b/PowerEditor/src/WinControls/ReadDirectoryChanges/ReadDirectoryChanges.cpp @@ -34,8 +34,8 @@ using namespace ReadDirectoryChangesPrivate; /////////////////////////////////////////////////////////////////////////// // CReadDirectoryChanges -CReadDirectoryChanges::CReadDirectoryChanges(int nMaxCount) - : m_Notifications(nMaxCount) +CReadDirectoryChanges::CReadDirectoryChanges() + : m_Notifications() { m_hThread = NULL; m_dwThreadId= 0; @@ -102,11 +102,3 @@ bool CReadDirectoryChanges::Pop(DWORD& dwAction, CStringW& wstrFilename) return true; } - -bool CReadDirectoryChanges::CheckOverflow() -{ - bool b = m_Notifications.overflow(); - if (b) - m_Notifications.clear(); - return b; -} diff --git a/PowerEditor/src/WinControls/ReadDirectoryChanges/ReadDirectoryChanges.h b/PowerEditor/src/WinControls/ReadDirectoryChanges/ReadDirectoryChanges.h index 33fad8d89..f1ae2c47b 100644 --- a/PowerEditor/src/WinControls/ReadDirectoryChanges/ReadDirectoryChanges.h +++ b/PowerEditor/src/WinControls/ReadDirectoryChanges/ReadDirectoryChanges.h @@ -99,8 +99,8 @@ namespace ReadDirectoryChangesPrivate /// { /// DWORD dwAction; /// CStringW wstrFilename; -/// changes.Pop(dwAction, wstrFilename); -/// wprintf(L"%s %s\n", ExplainAction(dwAction), wstrFilename); +/// while (changes.Pop(dwAction, wstrFilename)) +/// wprintf(L"%s %s\n", ExplainAction(dwAction), wstrFilename); /// } /// break; /// case WAIT_OBJECT_0 + _countof(handles): @@ -116,7 +116,7 @@ namespace ReadDirectoryChangesPrivate class CReadDirectoryChanges { public: - CReadDirectoryChanges(int nMaxChanges=1000); + CReadDirectoryChanges(); ~CReadDirectoryChanges(); void Init(); @@ -148,9 +148,6 @@ public: // "Push" is for usage by ReadChangesRequest. Not intended for external usage. void Push(DWORD dwAction, CStringW& wstrFilename); - // Check if the queue overflowed. If so, clear it and return true. - bool CheckOverflow(); - unsigned int GetThreadId() { return m_dwThreadId; } protected: diff --git a/PowerEditor/src/WinControls/ReadDirectoryChanges/ReadDirectoryChangesPrivate.cpp b/PowerEditor/src/WinControls/ReadDirectoryChanges/ReadDirectoryChangesPrivate.cpp index 05d82dfce..765160e53 100644 --- a/PowerEditor/src/WinControls/ReadDirectoryChanges/ReadDirectoryChangesPrivate.cpp +++ b/PowerEditor/src/WinControls/ReadDirectoryChanges/ReadDirectoryChangesPrivate.cpp @@ -123,11 +123,8 @@ VOID CALLBACK CReadChangesRequest::NotificationCompletion( // Can't use sizeof(FILE_NOTIFY_INFORMATION) because // the structure is padded to 16 bytes. - _ASSERTE(dwNumberOfBytesTransfered >= offsetof(FILE_NOTIFY_INFORMATION, FileName) + sizeof(WCHAR)); - - // This might mean overflow? Not sure. - if (!dwNumberOfBytesTransfered) - return; + _ASSERTE((dwNumberOfBytesTransfered == 0) || + (dwNumberOfBytesTransfered >= offsetof(FILE_NOTIFY_INFORMATION, FileName) + sizeof(WCHAR))); pBlock->BackupBuffer(dwNumberOfBytesTransfered); diff --git a/PowerEditor/src/WinControls/ReadDirectoryChanges/ThreadSafeQueue.h b/PowerEditor/src/WinControls/ReadDirectoryChanges/ThreadSafeQueue.h index d23aee5bc..f805b5c5a 100644 --- a/PowerEditor/src/WinControls/ReadDirectoryChanges/ThreadSafeQueue.h +++ b/PowerEditor/src/WinControls/ReadDirectoryChanges/ThreadSafeQueue.h @@ -32,52 +32,35 @@ template class CThreadSafeQueue : protected std::list { public: - CThreadSafeQueue(int nMaxCount) + CThreadSafeQueue() { - m_bOverflow = false; - - m_hSemaphore = ::CreateSemaphore( + m_hEvent = ::CreateEvent( NULL, // no security attributes - 0, // initial count - nMaxCount, // max count + FALSE, // auto reset + FALSE, // non-signalled NULL); // anonymous } ~CThreadSafeQueue() { - ::CloseHandle(m_hSemaphore); - m_hSemaphore = NULL; + ::CloseHandle(m_hEvent); + m_hEvent = NULL; } void push(C& c) { - CComCritSecLock lock( m_Crit, true ); - push_back( c ); - lock.Unlock(); - - if (!::ReleaseSemaphore(m_hSemaphore, 1, NULL)) { - // If the semaphore is full, then take back the entry. - lock.Lock(); - pop_back(); - if (GetLastError() == ERROR_TOO_MANY_POSTS) - { - m_bOverflow = true; - } + CComCritSecLock lock(m_Crit, true); + push_back(c); } + ::SetEvent(m_hEvent); } bool pop(C& c) { CComCritSecLock lock( m_Crit, true ); - - // If the user calls pop() more than once after the - // semaphore is signaled, then the semaphore count will - // get out of sync. We fix that when the queue empties. if (empty()) { - while (::WaitForSingleObject(m_hSemaphore, 0) != WAIT_TIMEOUT) - 1; return false; } @@ -87,30 +70,10 @@ public: return true; } - // If overflow, use this to clear the queue. - void clear() - { - CComCritSecLock lock( m_Crit, true ); - - for (DWORD i=0; i