From 51fb8678192eaba41e4a446f092c0027d786bd05 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Tue, 3 May 2011 23:31:29 +0000 Subject: [PATCH] Fix wxFileSystemWatcher::Remove() in wxMSW. Removing the path watched by wxFileSystemWatcher didn't do anything in wxMSW implementation so we still continued getting events for the changes to this path even after calling Remove(). Fix this by really implementing Remove() properly. Also add a unit test checking that we don't get any events after calling Remove(). See #12847. git-svn-id: https://svn.wxwidgets.org/svn/wx/wxWidgets/trunk@67691 c3d73ce0-8a6f-49c7-b76d-6d57e0e08775 --- include/wx/msw/private/fswatcher.h | 49 +++++++++++++++++++++++++++++- src/msw/fswatcher.cpp | 9 ++++-- tests/fswatcher/fswatchertest.cpp | 47 ++++++++++++++++++++++++++++ 3 files changed, 102 insertions(+), 3 deletions(-) diff --git a/include/wx/msw/private/fswatcher.h b/include/wx/msw/private/fswatcher.h index 99d2a21b17..c6cbdf3d34 100644 --- a/include/wx/msw/private/fswatcher.h +++ b/include/wx/msw/private/fswatcher.h @@ -104,7 +104,6 @@ private: wxDECLARE_NO_COPY_CLASS(wxFSWatchEntryMSW); }; - // ============================================================================ // wxFSWatcherImplMSW helper classes implementations // ============================================================================ @@ -156,6 +155,48 @@ public: return m_watches.insert(val).second; } + // Removes a watch we're currently using. Notice that this doesn't happen + // immediately, CompleteRemoval() must be called later when it's really + // safe to delete the watch, i.e. after completion of the IO operation + // using it. + bool ScheduleForRemoval(const wxSharedPtr& watch) + { + wxCHECK_MSG( m_iocp != INVALID_HANDLE_VALUE, false, "IOCP not init" ); + wxCHECK_MSG( watch->IsOk(), false, "Invalid watch" ); + + const wxString path = watch->GetPath(); + wxFSWatchEntries::iterator it = m_watches.find(path); + wxCHECK_MSG( it != m_watches.end(), false, + "Can't remove a watch we don't use" ); + + // We can't just delete the watch here as we can have pending events + // for it and if we destroyed it now, we could get a dangling (or, + // worse, reused to point to another object) pointer in ReadEvents() so + // just remember that this one should be removed when CompleteRemoval() + // is called later. + m_removedWatches.insert(wxFSWatchEntries::value_type(path, watch)); + m_watches.erase(it); + + return true; + } + + // Really remove the watch previously passed to ScheduleForRemoval(). + // + // It's ok to call this for a watch that hadn't been removed before, in + // this case we'll just return false and do nothing. + bool CompleteRemoval(wxFSWatchEntryMSW* watch) + { + wxFSWatchEntries::iterator it = m_removedWatches.find(watch->GetPath()); + if ( it == m_removedWatches.end() ) + return false; + + // Removing the object from the map will result in deleting the watch + // itself as it's not referenced from anywhere else now. + m_removedWatches.erase(it); + + return true; + } + // post completion packet bool PostEmptyStatus() { @@ -203,7 +244,13 @@ protected: } HANDLE m_iocp; + + // The hash containing all the wxFSWatchEntryMSW objects currently being + // watched keyed by their paths. wxFSWatchEntries m_watches; + + // Contains the watches which had been removed but are still pending. + wxFSWatchEntries m_removedWatches; }; diff --git a/src/msw/fswatcher.cpp b/src/msw/fswatcher.cpp index 28c268e062..bda04384b0 100644 --- a/src/msw/fswatcher.cpp +++ b/src/msw/fswatcher.cpp @@ -108,9 +108,9 @@ bool wxFSWatcherImplMSW::DoAdd(wxSharedPtr watch) } bool -wxFSWatcherImplMSW::DoRemove(wxSharedPtr WXUNUSED(watch)) +wxFSWatcherImplMSW::DoRemove(wxSharedPtr watch) { - return true; + return m_iocp.ScheduleForRemoval(watch); } // TODO ensuring that we have not already set watch for this handle/dir? @@ -216,6 +216,11 @@ bool wxIOCPThread::ReadEvents() wxLogTrace( wxTRACE_FSWATCHER, "[iocp] Read entry: path='%s'", watch->GetPath()); + // First check if we're still interested in this watch, we could have + // removed it in the meanwhile. + if ( m_iocp->CompleteRemoval(watch) ) + return true; + // extract events from buffer info our vector container wxVector events; const char* memory = static_cast(watch->GetBuffer()); diff --git a/tests/fswatcher/fswatchertest.cpp b/tests/fswatcher/fswatchertest.cpp index e75a1ccbe4..f28fee917e 100644 --- a/tests/fswatcher/fswatchertest.cpp +++ b/tests/fswatcher/fswatchertest.cpp @@ -412,6 +412,8 @@ private: CPPUNIT_TEST( TestEventAccess ); #endif // __WXMSW__ #endif // !wxHAS_KQUEUE + + CPPUNIT_TEST( TestNoEventsAfterRemove ); CPPUNIT_TEST_SUITE_END(); void TestEventCreate(); @@ -420,6 +422,8 @@ private: void TestEventModify(); void TestEventAccess(); + void TestNoEventsAfterRemove(); + DECLARE_NO_COPY_CLASS(FileSystemWatcherTestCase) }; @@ -605,3 +609,46 @@ void FileSystemWatcherTestCase::TestEventAccess() EventTester tester; tester.Run(); } + +void FileSystemWatcherTestCase::TestNoEventsAfterRemove() +{ + class EventTester : public EventHandler, + public wxTimer + { + public: + EventTester() + { + // We need to use an inactivity timer as we never get any file + // system events in this test, so we consider that the test is + // finished when this 1s timeout expires instead of, as usual, + // stopping after getting the file system events. + Start(1000, true); + } + + virtual void GenerateEvent() + { + m_watcher->Remove(EventGenerator::GetWatchDir()); + CPPUNIT_ASSERT(eg.CreateFile()); + } + + virtual void CheckResult() + { + CPPUNIT_ASSERT( m_events.empty() ); + } + + virtual wxFileSystemWatcherEvent ExpectedEvent() + { + CPPUNIT_FAIL( "Shouldn't be called" ); + + return wxFileSystemWatcherEvent(wxFSW_EVENT_ERROR); + } + + virtual void Notify() + { + SendIdle(); + } + }; + + EventTester tester; + tester.Run(); +} -- 2.45.2