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

diff --git a/include/wx/msw/private/fswatcher.h b/include/wx/msw/private/fswatcher.h
index 99d2a21b1746f2cc8c44debeed2149d5af7ec337..c6cbdf3d34c67e79d4dd5640b10707b2bc2df0b3 100644 (file)
--- 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<wxFSWatchEntryMSW>& 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 28c268e062b4f31285143295b006f5432c49a1ef..bda04384b06f5657e8bb53eee5640a324f5af2b6 100644 (file)
--- a/src/msw/fswatcher.cpp
+++ b/src/msw/fswatcher.cpp
@@ -108,9 +108,9 @@ bool wxFSWatcherImplMSW::DoAdd(wxSharedPtr<wxFSWatchEntryMSW> watch)
 }
 
 bool
-wxFSWatcherImplMSW::DoRemove(wxSharedPtr<wxFSWatchEntryMSW> WXUNUSED(watch))
+wxFSWatcherImplMSW::DoRemove(wxSharedPtr<wxFSWatchEntryMSW> 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<wxEventProcessingData> events;
     const char* memory = static_cast<const char*>(watch->GetBuffer());

diff --git a/tests/fswatcher/fswatchertest.cpp b/tests/fswatcher/fswatchertest.cpp
index e75a1ccbe483935bacb5c681fff1bf08ce6dfd8d..f28fee917e1111a1516a4bd5ece4f283edf05bba 100644 (file)
--- 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();
+}