Opened 3 years ago

Last modified 6 months ago

#12847 confirmed defect

Fix Remove{All,Tree}() and better error checking reporting in wxFileSystemWatcher

Reported by: PB Owned by:
Priority: normal Milestone:
Component: wxMSW Version: stable-latest
Keywords: wxFileSystemWatcher Cc: exsudat@…
Blocked By: Blocking:
Patch: no

Description

I've run into few issues when playing with fswatcher sample on MSW (Windows 7 and Windows 7's XP mode).

Issue 1
If you remove a path, then it can not be added again.
How to reproduce:

  1. Run the fswatcher sample
  2. Add "C:\"
  3. Remove "C:\"
  4. Attempt to add "C:\" again: wxFileSystemWatcherBase::Add() in "src\common\fswatchercmn.cpp" returns false from the last line: "m_watches.insert(val).second;"

The issue manifests also on non-root folders.

Issue 2
If you add a path, it seems ALL the folders (I haven't tried with files) below the path are being watched no matter how deep they are within folder hierarchy. E.g. when adding "C:\", I got notified even for "C:\Dev\Desktop\wxWidgets-SVN\samples\fswatcher\vc_mswud\test.tst". Is this how it is supposed to work - documentation reads "When adding a directory, immediate children will be watched as well."?
The issue manifests also on non-root folders.

Issue 3
wxFileSystemWatcher documentation doesn't match its implementation.
Functions GetWatchedPathCount, OnChange, OnError and OnWarning are not present in the actual code.


By the way, should I continue reporting these low priority issues I have no solutions for?

Attachments (2)

fswatcher18.patch download (37.0 KB) - added by PB 3 years ago.
Fixes Issue 1 and 2 and for the time being also 4 and 5
12847_overflow.patch download (6.3 KB) - added by roberto 9 months ago.
patch that creates an overflow warning type to wxFileSystemWatcherEvent

Download all attachments as: .zip

Change History (34)

comment:1 Changed 3 years ago by PB

Regarding issue 3: function wxFileSystemWatcher::GetWatchedPathCount() is there, my bad. Sorry for that.

comment:2 Changed 3 years ago by PB

Mmm, today is really not my day. Reference Manual lists the function as wxFileSystemWatcher::GetWatchedPathCount() but it is actually implemented as wxFileSystemWatcher::GetWatchedPathsCount() (notice the "s").

comment:3 in reply to: ↑ description ; follow-up: Changed 3 years ago by vadz

  • Component changed from base to wxMSW
  • Keywords wxFileSystemWatcher added
  • Status changed from new to confirmed
  • Summary changed from Issues with wxFileSystemWatcher to Removing and adding back a path to wxFileSystemWatcher doesn't work

Replying to PB:

I've run into few issues when playing with fswatcher sample on MSW (Windows 7 and Windows 7's XP mode).

Issue 1
If you remove a path, then it can not be added again.

This actually doesn't seem to be low priority as it's a rather bad problem if you need to do this. It would be nice to debug why does it happen but I don't have time to do it right now unfortunately. It looks like removing the path doesn't really work, in fact...

Issue 2
If you add a path, it seems ALL the folders (I haven't tried with files) below the path are being watched no matter how deep they are within folder hierarchy.

This is not the expected behaviour as only AddTree() should do this and AFAICS the sample doesn't call it.

In fact I just checked under wxGTK and it works correctly there so this definitely looks like a problem with wxMSW implementation.

Issue 3
wxFileSystemWatcher documentation doesn't match its implementation.
Functions GetWatchedPathCount, OnChange, OnError and OnWarning are not present in the actual code.

I fixed the typo in the first one name. As for the rest of them, we've apparently decided to remove these virtual methods (see r61484 and r61476) although I don't remember why to be honest. Anyhow, I'll remove the documentation for them as well, thanks for noticing.

By the way, should I continue reporting these low priority issues I have no solutions for?

Yes, of course, please do. We won't always have time to look into them immediately so if you can debug them further yourself this would, of course, be very welcome but if not please still record them in the Trac (just make sure not to create duplicate entries) so that we are at least aware about them. Thanks!

comment:4 Changed 3 years ago by VZ

(In [66603]) Correct wxFileSystemWatcher::GetWatchedPathsCount() documentation.

The method name was misspelt.

Also improve the description slightly.

See #12847.

comment:5 Changed 3 years ago by VZ

(In [66604]) Remove wxFileSystemWatcher::OnXXX() virtual methods documentation.

These functions don't exist any more (they had been present initially but were
removed in r61484 and r61476 for Unix and MSW respectively) so don't document
them.

See #12847.

comment:6 in reply to: ↑ 3 Changed 3 years ago by PB

Replying to vadz:

Replying to PB:

I've run into few issues when playing with fswatcher sample on MSW (Windows 7 and Windows 7's XP mode).

Issue 1
If you remove a path, then it can not be added again.

This actually doesn't seem to be low priority as it's a rather bad problem if you need to do this. It would be nice to debug why does it happen but I don't have time to do it right now unfortunately. It looks like removing the path doesn't really work, in fact...

I am looking into this and there are some oddities with MSW implementation, I don't know if I can fix it, but I will try.

comment:7 Changed 3 years ago by PB

Re: Issue 1
Vadim, you were indeed correct – the watch is not being properly removed. When a watch is added the add call is chained through wxFileSystemWatcherBase::Add() --> wxFSWatcherImpl::Add() --> wxFSWatcherImplMSW::DoAdd() --> wxIOCPService::Add(). Each of these classes except wxFSWatcherImplMSW (which inherits from wxFSWatcherImpl) keeps its own list of watches. When you attempt to remove a watch, the the call is delegated further down, so it goes like this: wxFileSystemWatcherBase::Remove --> wxFSWatcherImpl::Remove --> wxFSWatcherImplMSW::DoRemove, the first two classes removing the watch from their list before propagating the call. The thing is that wxFSWatcherImplMSW::DoRemove just returns true instead of actually doing something. By the way, wxFSWatcherImpl::RemoveAll() only clears the list of its watches but doesn’t attempt to really remove them and wxFSWatcherImplMSW class doesn’t override this function so there’s nothing going on there either. I assume there’s a good reason why those three classes keep separate watch lists instead of just querying the underlying objects for the information, but wouldn’t it make more sense to actually remove the watch from the list only if the delegated Remove() call succeeded?
The real problem is that wxIOCPService class doesn’t provide a way how to actually tell the Windows we are not interested in the watching given folder anymore. When I added wxIOCPService::Remove method and started testing it using fswatcher sample, I found out that sometimes (highly randomly) I stop getting file change events. I thought it might be a bug I introduced with my „fix“ (which I threw away anyway as it was not an ideal solution), but the issue manifests even with original SVN code. Sometimes I get only as few as 20 notifies or so, sometimes it stops around 800, I often get over 20k and at least once I went close to 65k – I usually track whole „C:\“ ditree (relying on current buggy behaviour) while building wxWidgets. It looks like it always ends to be waiting forever on GetQueuedCompletionStatus() in wxIOCPService::GetStatus(). I observed this behaviour on two different computers, one running W7 Pro and the other WXP Pro. It seems that it’s more susceptible to this when the debug build of the sample is run under MSVC (2008 Express Edition) debugger, I thought it might be because of massive amount of wxLogTraces it generated but sometimes it stops even when there’s not that many notifies being generated, but sometimes it stops receiving them even after uncommenting those two wxLog::AddTraceMask() lines in the sample source. I wonder if anyone else also can reproduce this.

Re: Issue 2
wxFSWatcherImplMSW::DoSetUpWatch() always calls ReadDirectoryChangesW() WinAPI function with the parameter bWatchSubtree set to TRUE. According to MSDN “If this parameter is TRUE, the function monitors the directory tree rooted at the specified directory. If this parameter is FALSE, the function monitors only the directory specified by the hDirectory parameter.“ Thus, I believe that bWatchSubtree should be always set to FALSE. At first it seems this parameter could be very useful in MSW implementation of wxFileSystemWatcher::AddTree() but I don’t think that would be easily doable, as there’s no way to set the filter for files in the dirtree when calling ReadDirectoryChangesW(). There would have to be either two code paths in wxFileSystemWatcher::AddTree() when there’s no file filter passed to AddTree() use ReadDirectoryChangesW() with bWatchSubtree set to TRUE; if there’s a filter, fall back to the current implementation. Or; rely on Windows watching the whole dirtree and filter out by ourselves the notifies on files we are not interested in. Neither way seems to be worth the trouble to me.

Re: Issue 3
While the OnXXX() methods were removed from class declaration in the documentation, there are still mentioned several times (including example code). Speaking about documentation: wxFileSystemWatcher::Add() description reads as if we could watch not only a folder but also a file. I haven't tested it but I don’t think that would work with current MSW implementation which can watch only the contents of a directory. If we want to track a single we would have to track changes in its directory and filter out notifies about the files we are not interested in.

comment:8 follow-up: Changed 3 years ago by PB

  • Patch set

I have spent some more time digging into the "randomly stop getting notifies" issue I mentioned in my previous post. I think I found the culprit and it was easy to fix.

bool wxIOCPThread::ReadEvents()
{
    unsigned long count = 0;
    wxFSWatchEntryMSW* watch = NULL;
    OVERLAPPED* overlapped = NULL;
    if (!m_iocp->GetStatus(&count, &watch, &overlapped))
        return true; // error was logged already, we don't want to exit

    // this is our exit condition, so we return false
    if (!count && !watch && !overlapped)
        return false;

   // in case of spurious wakeup
    if (!count || !watch)
        return true;
    
    more code follows ...

I believe the bug is in the last two lines I posted. According to what I dug up on the internet, sometimes when there's too many notifies in very short time, the buffer passed to ReadDirectoryChangesW() gets overflowed. It seems that in that case GetQueuedCompletionStatus() doesn't return an error but lpNumberOfBytes and lpCompletionKey are invalid. lpOverlapped is still valid and we can find a watch using this parameter and reissue the ReadDirectoryChangesW() again. The current implementation just returns from the function without reissuing the read which means we don't get any more notifies.
Unfortunately, once this was fixed another issue appeared. If there are MANY notifies being posted to the wxWidgets event queue in SHORT amount of time, the queue gets flooded and the application stops responding for some time, struggling to process the events. E.g. when watching "C:\" ditree and building wxWidgets on this drive, the sample was still processing wxFileSystemWatcherEvents minutes after the build has finished - I believe the build generated over 60,000 events. When obtaining whole wxWidgets tree from SVN about 190,000 events were generated in few minutes. I don't have any idea how long the sample took to process it, but when I came back after an hour or so, it was done and still alive. I think at least once I even got stack overflow when running debug build under MSVC, I believe that extensive logging done by the sample might have "helped". I maybe missing something here, but I don't see a way how the event handler can tell wxFSWatcher that they have enough events to process so it should send new ones for the time being. Of course, throwing events away is a bad idea but maybe still better than application becoming virtually unusable? By the way, it's not guaranteed we get really get notified about all the changes in tracked folder, see various complaining posts about that on the internet.
Of course, all this can happen because there's a bug in my fix, but I don't think it's very likely.
Attached patch hopefully fixes the issue described in this post, but it's not intended to be merged into wxWidgets trunk, only for review for those who may be interested in it. This patch also includes fix for issue 2, calling ReadDirectoryChangesW() with parameter bWatchSubtree set to FALSE - I actually haven't tested this one yet.

So far, I have made no progress on issue 1, as I have been attempting to fix the hard to reproduce bug described above.

comment:9 in reply to: ↑ 8 ; follow-up: Changed 3 years ago by PB

Replying to PB:
The patch itself, don't know how to attach it to a post :S

Index: samples/fswatcher/fswatcher.cpp
===================================================================
--- samples/fswatcher/fswatcher.cpp (revision 66659)
+++ samples/fswatcher/fswatcher.cpp (working copy)
@@ -210,7 +210,7 @@

wxTextCtrl *headerText = new wxTextCtrl(this, wxID_ANY, "",

wxDefaultPosition, wxDefaultSize,
wxTE_READONLY);

  • wxString h = wxString::Format(LOG_FORMAT, "event", "path", "new path");

+ wxString h = wxString::Format(LOG_FORMAT, "#", "type", "path", "new path");

headerText->SetValue(h);


event console

@@ -392,7 +392,10 @@

void MyFrame::LogEvent(const wxFileSystemWatcherEvent& event)
{

+ static unsigned count = 0;
+

wxString entry = wxString::Format(LOG_FORMAT + "\n",

+ wxString::Format("%u", ++count),

GetFSWEventChangeTypeName(event.GetChangeType()),
event.GetPath().GetFullPath(),
event.GetNewPath().GetFullPath());

Index: src/msw/fswatcher.cpp
===================================================================
--- src/msw/fswatcher.cpp (revision 66659)
+++ src/msw/fswatcher.cpp (working copy)
@@ -37,8 +37,10 @@

bool SetUpWatch(wxFSWatchEntryMSW& watch);


  • void SendEvent(wxFileSystemWatcherEvent& evt);

+ wxFSWatchEntryMSW* FindWatchWithOverlapped(OVERLAPPED* ovr);

+ void SendEvent(wxFileSystemWatcherEvent& evt);
+

protected:

bool Init();


@@ -128,6 +130,22 @@

return DoSetUpWatch(watch);

}


+
+wxFSWatchEntryMSW* wxFSWatcherImplMSW::FindWatchWithOverlapped(OVERLAPPED* ovr)
+{
+ wxSharedPtr<wxFSWatchEntryMSW> watch;
+ wxFSWatchEntries::iterator it;
+
+ for (it = m_watches.begin(); it != m_watches.end(); it++) {
+ watch = it->second;
+ if (watch->GetOverlapped() == ovr) {
+ return watch.get();
+ }
+ }
+ return NULL;
+}
+
+

void wxFSWatcherImplMSW::SendEvent(wxFileSystemWatcherEvent& evt)
{

called from worker thread, so posting event in thread-safe way

@@ -138,7 +156,7 @@

{

int flags = Watcher2NativeFlags(watch.GetFlags());
int ret = ReadDirectoryChangesW(watch.GetHandle(), watch.GetBuffer(),

  • wxFSWatchEntryMSW::BUFFER_SIZE, TRUE,

+ wxFSWatchEntryMSW::BUFFER_SIZE, FALSE,

flags, NULL,
watch.GetOverlapped(), NULL);

if (!ret)

@@ -209,32 +227,36 @@

if (!count && !watch && !overlapped)

return false;


  • in case of spurious wakeup
if (!count
!watch)
  • return true;

+

+ if (!count
!watch) { buffer overflowed, notifies were discarded but we have to reissue the watch

+ wxASSERT(HasOverlappedIoCompleted(overlapped)); should be always true
+ watch = m_service->FindWatchWithOverlapped(overlapped);
+ wxCHECK_MSG(watch, true, "Failed to find watch its buffer overflowed");
+ } else { process notifies

  • wxLogTrace( wxTRACE_FSWATCHER, "[iocp] Read entry: path='%s'",
  • watch->GetPath());

+ wxLogTrace( wxTRACE_FSWATCHER, "[iocp] Read entry: path='%s'",
+ watch->GetPath());

  • extract events from buffer info our vector container
  • wxVector<wxEventProcessingData> events;
  • const char* memory = static_cast<const char*>(watch->GetBuffer());
  • int offset = 0;
  • do
  • {
  • const FILE_NOTIFY_INFORMATION* e =
  • static_cast<const FILE_NOTIFY_INFORMATION*>((const void*)memory);

+ extract events from buffer info our vector container
+ wxVector<wxEventProcessingData> events;
+ const char* memory = static_cast<const char*>(watch->GetBuffer());
+ int offset = 0;
+ do
+ {
+ const FILE_NOTIFY_INFORMATION* e =
+ static_cast<const FILE_NOTIFY_INFORMATION*>((const void*)memory);

  • events.push_back(wxEventProcessingData(e, watch));

+ events.push_back(wxEventProcessingData(e, watch));

  • offset = e->NextEntryOffset;
  • memory += offset;

+ offset = e->NextEntryOffset;
+ memory += offset;
+ }
+ while (offset);
+
+ process events
+ ProcessNativeEvents(events);

}

  • while (offset);


  • process events
  • ProcessNativeEvents(events);

-

reissue the watch. ignore possible errors, we will return true anyway
(void) m_service->SetUpWatch(*watch);


comment:10 in reply to: ↑ 9 Changed 3 years ago by PB

Third time's the charm, this is the correct patch, the previous was diffed against wrong fwwatcher sample source on line 59. Not that the changed sample is necessary, it only adds event's ordinal numbers so it's easier to see how many file change events were processed.

Index: samples/fswatcher/fswatcher.cpp
===================================================================
--- samples/fswatcher/fswatcher.cpp	(revision 66659)
+++ samples/fswatcher/fswatcher.cpp	(working copy)
@@ -59,7 +59,7 @@
     const static wxString LOG_FORMAT; // how to format events
 };
 
-const wxString MyFrame::LOG_FORMAT = " %-12s %-36s    %-36s";
+const wxString MyFrame::LOG_FORMAT = "%-6s %-12s %-36s    %-36s";
 
 // Define a new application type, each program should derive a class from wxApp
 class MyApp : public wxApp
@@ -210,7 +210,7 @@
     wxTextCtrl *headerText = new wxTextCtrl(this, wxID_ANY, "",
                                             wxDefaultPosition, wxDefaultSize,
                                             wxTE_READONLY);
-    wxString h = wxString::Format(LOG_FORMAT, "event", "path", "new path");
+    wxString h = wxString::Format(LOG_FORMAT, "#", "type", "path", "new path");
     headerText->SetValue(h);
 
     // event console
@@ -392,7 +392,10 @@
 
 void MyFrame::LogEvent(const wxFileSystemWatcherEvent& event)
 {
+    static unsigned count = 0;
+    
     wxString entry = wxString::Format(LOG_FORMAT + "\n",
+                            wxString::Format("%u", ++count),
                             GetFSWEventChangeTypeName(event.GetChangeType()),
                             event.GetPath().GetFullPath(),
                             event.GetNewPath().GetFullPath());
Index: src/msw/fswatcher.cpp
===================================================================
--- src/msw/fswatcher.cpp	(revision 66659)
+++ src/msw/fswatcher.cpp	(working copy)
@@ -37,8 +37,10 @@
 
     bool SetUpWatch(wxFSWatchEntryMSW& watch);
 
-    void SendEvent(wxFileSystemWatcherEvent& evt);
+    wxFSWatchEntryMSW* FindWatchWithOverlapped(OVERLAPPED* ovr);
 
+    void SendEvent(wxFileSystemWatcherEvent& evt);    
+
 protected:
     bool Init();
 
@@ -128,6 +130,22 @@
     return DoSetUpWatch(watch);
 }
 
+
+wxFSWatchEntryMSW* wxFSWatcherImplMSW::FindWatchWithOverlapped(OVERLAPPED* ovr)
+{
+    wxSharedPtr<wxFSWatchEntryMSW> watch;
+    wxFSWatchEntries::iterator it;
+
+    for (it = m_watches.begin(); it != m_watches.end(); it++) {
+        watch = it->second;        
+        if (watch->GetOverlapped() == ovr) {                    
+            return watch.get();            
+        }     
+    }
+    return NULL;
+}
+
+
 void wxFSWatcherImplMSW::SendEvent(wxFileSystemWatcherEvent& evt)
 {
     // called from worker thread, so posting event in thread-safe way
@@ -138,7 +156,7 @@
 {
     int flags = Watcher2NativeFlags(watch.GetFlags());
     int ret = ReadDirectoryChangesW(watch.GetHandle(), watch.GetBuffer(),
-                                    wxFSWatchEntryMSW::BUFFER_SIZE, TRUE,
+                                    wxFSWatchEntryMSW::BUFFER_SIZE, FALSE,
                                     flags, NULL,
                                     watch.GetOverlapped(), NULL);
     if (!ret)
@@ -209,32 +227,36 @@
     if (!count && !watch && !overlapped)
         return false;
 
-    // in case of spurious wakeup
-    if (!count || !watch)
-        return true;
+    
+    if (!count || !watch) { // buffer overflowed, notifies were discarded but we have to reissue the watch
+        wxASSERT(HasOverlappedIoCompleted(overlapped)); // should be always true
+        watch = m_service->FindWatchWithOverlapped(overlapped);        
+        wxCHECK_MSG(watch, true, "Failed to find watch its buffer overflowed");
+    } else { // process notifies
 
-    wxLogTrace( wxTRACE_FSWATCHER, "[iocp] Read entry: path='%s'",
-                watch->GetPath());
+        wxLogTrace( wxTRACE_FSWATCHER, "[iocp] Read entry: path='%s'",
+                    watch->GetPath());
 
-    // extract events from buffer info our vector container
-    wxVector<wxEventProcessingData> events;
-    const char* memory = static_cast<const char*>(watch->GetBuffer());
-    int offset = 0;
-    do
-    {
-        const FILE_NOTIFY_INFORMATION* e =
-              static_cast<const FILE_NOTIFY_INFORMATION*>((const void*)memory);
+        // extract events from buffer info our vector container
+        wxVector<wxEventProcessingData> events;
+        const char* memory = static_cast<const char*>(watch->GetBuffer());
+        int offset = 0;
+        do
+        {
+            const FILE_NOTIFY_INFORMATION* e =
+                  static_cast<const FILE_NOTIFY_INFORMATION*>((const void*)memory);
 
-        events.push_back(wxEventProcessingData(e, watch));
+            events.push_back(wxEventProcessingData(e, watch));
 
-        offset = e->NextEntryOffset;
-        memory += offset;
+            offset = e->NextEntryOffset;
+            memory += offset;
+        }
+        while (offset);
+
+        // process events
+        ProcessNativeEvents(events);
     }
-    while (offset);
 
-    // process events
-    ProcessNativeEvents(events);
-
     // reissue the watch. ignore possible errors, we will return true anyway
     (void) m_service->SetUpWatch(*watch);

comment:11 follow-up: Changed 3 years ago by vadz

  • Milestone set to 2.9.2
  • Owner set to vadz
  • Status changed from confirmed to accepted

Thanks a lot for looking into this, I'll try to apply the patch and retest soon.

comment:12 in reply to: ↑ 11 Changed 3 years ago by PB

  • Patch unset

Replying to vadz:

Thanks a lot for looking into this, I'll try to apply the patch and retest soon.

I guess that wall of text I've produced was way too long. :) The patch I've provided does NOT fix the issue mentioned in the ticket title, it's supposed to fix issue 2 and another bug (file change events stop being sent) I've encountered while testing solution for issue 1.
I expect to have fix for issue 1 ready in two weeks max, maybe sooner depending on my schedule. So it might be better to leave fswatcher alone for the time being and wait till I have fixed the watches removal (assuming all goes well, it's ASIO after all). I'm sorry for the confusion, could you please uncheck "Patch" in my posts and change the ticket status back to "Confirmed"? Thank you.

comment:13 Changed 3 years ago by PB

  • Patch set

Review of issues I found when playing with wxFileSystemWatcher on Windows. I’ve attempted to fix some in the patch attached below. If you decide to use some portions of it, please perform careful code review and testing, it's just few lines after all. I suppose the additions provided by GSoC programmers are trusted and not reviewed or tested but I’m a very green programmer, this was my first experience with asynchronous IO and I’m not exactly a smart guy to begin with.

Issue 1: Removing and adding back a path to wxFileSystemWatcher doesn't work
The watch was not properly removed so no wonder it could not be readded.
Fixed: Yes.

Issue 2: When adding a directory using Add() whole directory subtree was watched
ReadDirectoryChangesW was always called with parameter bWatchSubtree
set to true
Fixed: Yes

Issue 3: Improve wxFSWatcher documentation

  • Remove all remaining references to OnChange(), OnWarning() and OnError() methods, including the example
  • wxFileSystemWatcher::AddTree() is not a pure virtual method. It has an implementation, which BTW contrary to the Reference manual adds only directories and entirely ignores filter parameter.
  • List wxFSWFlags
  • Documentation hints you can watch individual files, at least on MSW this is not possible.
  • It is mentioned there is an OSX implementation, is there really one? I can’t find it in the sources, but maybe I’m looking in wrong places.
  • Mention more platform specific gotchas, e.g. on MSW it’s file notifies buffer overflow, long vs short path names.

Fixed: NO

Issue 4: Overflow of buffer supplied to ReadDirectoryChangesW is not handled properly
If the buffer supplied to ReadDirectoryChangesW overflows (can and does happen), the application programmer is not notified that some file change events have been lost and full directory rescan is needed. Worse, the watch is put in the limbo and no new file events for its directory are going to be sent anymore
Fixed: Yes, but the way how the app programmer is notified should be improved and it has to be done accordance with other OSes. Currently I just send a warning event (if the watch flags have warning set) with a “Full directory rescan needed” error message. It would be probably best to add error code data member to the file change event, needs to be unified across the platforms, I can’t do that being just a Windows guy.

Issue 5: If there’s error during reading/processing native file notifies, the error is not propagated to application prorammer.
If an error happens during IO, no file change event with an error flag is send so the application programmer doesn’t know that the watch is not active anymore (The error is reported only to the user via wxLogSysError).
This can happen e.g. if a user attempts to delete a watched directory.
Fixed: Yes, but the way how the app programmer is notified should be improved see fix for Issue 4.

Issue 6: Wrong file names are supplied in file change events if the watched directory was renamed
Description
If the watched directory is renamed, the change in name is not reflected in the watch and issued file change events still have old directory name, i.e. the application programmer receives file change events about files that do not exist.

How to reproduce:

  1. Create a folder named “C:\AAA”
  2. Run the fswatcher sample and add the folder to watched paths
  3. Rename the folder to “C:\BBB”, i.e. there’s no “C:\AAA” folder anymore
  4. In the “C:\BBB” folder create file called “test.txt”
  5. As the sample shows, an event was generated for file “C:\AAA\test.txt”

Reason
Problem is that Windows doesn’t issue notify about the rename of the watched folder itself, thus the watch is oblivious to the name change. Windows send notifies with names relative to the watched folder, the wxWidgets watch prepends those names with a path it had when it was created.

Solution
I don’t have any. Few solutions that comes to my mind:

  • Prevent user from renaming the watched directory. I don’t think this is a good idea. On the other hand, the user can’t really delete the watched directory either while it’s still in use by the watch.
  • Creating a separate internal watch that watches the watched directory and notifies it about its rename would probably be rather complicated and not 100% reliable anyway.
  • When processing notifies issued by the system, the watch is to query the OS for the directory name using the directory handle it has. Performance hit this might introduce would probably (uneducated guess) not be significant. The implementation is likely to depend on Windows’ internal function NtQueryObject, which can be removed from new Windows versions (removal not very likely IMO, uneducated guess again), but still could be a problem to rely on such a function. As I can see it the main problem is that we can still fire a few invalid events because we don’t know exactly when the rename happened. If it happens after file notifies being queued, we will query and attach the new name directory name despite that the old one was probably still in use and valid at the time the event happened. In most cases the user actually may want this behaviour because by the time he is to process the file he was notified about, it will have a new path, but there may be cases where such behaviour is highly undesirable.

Also currently there’s no way to propagate the change in the name of the watched directory to: a. classes above the low level one which actually does all the work (IMO one more reason why those classes shouldn’t keep their own watch list and just query the underlying class); b. the application programmer, but this would be easy, either send the rename as a regular file change event or better yet, create a new change type.

  • Mimic what Windows does and send the paths relative to the watched directory. User would have to set an id for each watched directory (e.g. AddDirectory(path, id, flags) ) and the id would be set in the file change event. Pros: The simplest and most reliable solution. Cons: Puts the burden on application programmer. Will have to be implemented the same way for all OSes.

Fixed: NO

Proposal for changing wxFileSystemWatcher interface

  • Merge methods Add(const wxFileName& path, int events=wxFSW_EVENT_ALL)

and AddTree(const wxFileName& path, int events=xFSW_EVENT_ALL, const wxString& filter = wxEmptyString ) methods into one
Add(const wxString& directoryName, int flags, bool watchWholeSubTree=false)
I don’t know if it is possible to add a single watch for whole subtree on all OSes, like it is on MSW. This way it is much more efficient, easier to implement and minimizes issues like issue 5. Also (under Windows), directories can’t be removed while being watched – users don’t like when folders cannot be deleted. Watching individual files is not implemented on MSW anyway, neither is filtering by a file mask. MSW don’t support this - the underlying function can track only the contents of a directory. It would to be done manually, watching the parent directory and filtering out notifies about files we are not interested in.

  • Remove the RemoveTree() method which won’t be needed anymore, see above.

Make wxFSWatchInfo class public and add the following member to it (see above)
bool IsWatchingWholeSubTree(const wxString& directoryName) const

Add the following method (currently there’s no way how to obtain the flags directory watch has):
bool wxFileSystemWatcher::GetWatchedDirectoryInfo(wxFSWatchInfo& info) const


TL;DR: I believe there’s quite a few issues that have to be dealt with if wxFileSystemWatcher is to be included in the stable branch of wxWidgets.

BTW: Maybe someone should thoroughly test the two other native implementations of file system watching too.

Changed 3 years ago by PB

Fixes Issue 1 and 2 and for the time being also 4 and 5

comment:14 Changed 3 years ago by MoonKid

  • Cc exsudat@… added

comment:15 Changed 3 years ago by MoonKid

  • Cc exsudat@… removed

comment:16 Changed 3 years ago by MoonKid

  • Cc exsudat@… added

comment:17 follow-up: Changed 3 years ago by vadz

Thanks a lot for your work, I've finally got to looking at it in details. Let me answer your points from comment:13 one by one:

  1. Removing a path doesn't work: This is obviously a bug and I'd be glad to apply this part of the patch to fix it. The problem is that it's not that easy to extract just this part from the patch which contains many unrelated fixes and, annoyingly, many insignificant changes to whitespace. But, again, I'll just need to do it. I'll also need to understand the code better and, in particular, understand in which threads are different functions executed to be sure that we don't have any problems here, it doesn't seem obvious at first glance. If you have any explanations about this part of your patch, I'd like to hear them.
  2. Add() behaved as AddTree(): The fix here is trivial but suboptimal as we really don't want to use the generic AddTree() implementation for MSW and add watches for all subdirectories when we can just call ReadDirectoryChangesW() with the bRecursive = TRUE. It would be really much nicer to override AddTree() in wxMSWFileSystemWatcher to do it. If you could make a patch doing this, it would be great, otherwise, again, I'll try to do it myself.
  3. Improve documentation: Can't but agree with this one, it was somehow forgotten in the final and hectic (as usual) days of the project. I do recall the reason for removing OnXXX() methods now though: the problem is that these methods were called in the context of another thread in wxMSW and in the context of the main thread elsewhere which made writing portable code overriding them very difficult. I don't think this problem can be solved so, even though I'd prefer to have the methods which are significantly more efficient than events, we will need to remove them from documentation instead. As for the other points, watching individual files should, ideally, work too and it could be implemented by monitoring the parent directory and filtering the changes inside wx but this isn't done now. Also, we do have OS X implementation, it's the one using kqueue. But it has its own problems and limitations...
  4. ReadDirectoryChangesW() buffer overflow: This problem has just completely escaped our attention when writing this code :-( We do need to generate a warning for this but it needs to be somehow specifically recognizable as a warning about buffer overflow without doing any string matching (FWIW we use warning "Event queue overflowed" under Linux right now). This will require adding "warning type" field to the event object, I guess. I also think that such warnings should be always generated as it's simply not safe to ignore them. Independently of this I think we should increase the buffer size to 64KB to at least decrease the frequency of this problem. Is there any reason not to? Finally, MSDN seems to indicate that we're supposed to get ERROR_NOTIFY_ENUM_DIR when the buffer overflows but your code doesn't seem to check for it -- why not? 1. Error propagation: This should also be fixed but it's not clear to me what exactly happens here. I.e. does deleting the watched directory permanently invalidate the watch?
  5. Renaming watched directory: I don't see any good solution to this neither. For now I think all we can do is to document this and let the application deal with it, either by verifying the directory existence or by watching its parent or in some other way.
  6. Merge Add() and AddTree(): This isn't a good idea as MSW can implement both of them efficiently (see the second item) but other ports can't. This discrepancy was the reason for separating these two methods and I still think it was the right thing to do.

It would be great to fix at least the first three items soon, before we all forget about them again. If you can make a separate small patch just for the second item and, maybe, for the fourth one, it would be great. As I said, I'll take care of the first one and will also update the documentation.

Thanks again for your help with this!

comment:18 in reply to: ↑ 17 Changed 3 years ago by PB

Replying to vadz:

I'm sorry, I have (at least for the time being) stopped using wxWidgets but let me address at least few of your questions.

I apologize for not obeying the "one patch per issue" rule, but IIRC the vast majority of changes in my patch aside from the sample, as scattered as they might be, is related to issue 1. Of course, there's a fix to issue 2, but that's just one-word change (TRUE -> FALSE in ReadDirectoryChangesW call) and warning and error handling blocks in wxIOCPThread::ReadEvents() related to issues 4 and 5. I also apologize for the annoying whitespace changes.

Add() vs AddTree()
[As I wrote before, I'm not familiar with fswatcher implementations for other OSes.]
So if I understand it correctly, if you call the generic AddTree() on non-MSW OS it adds individual watches for all directories in the directory tree (as noted before, the current wxFileSystemWatcherBase::AddTree() implementation ignores the filter and adds only directories, not files, I don't see this method overridden anywhere). How is the application programmer supposed to remove the watch when he doesn't know about those countless watches that can be created behind his back?

On the other hand, implementing efficient version of AddTree() for MSW should be very easy. I won't do that now, because this would affect more than one file - a new member variable would have to be added to wxFSWatchEntryMSW class for keeping track if we are watching just a directory or whole directory tree and being passed to repeated ReadDirectoryChangesW calls. You seem to have your hands full with just the current patch.

File notifies buffer overflow
I've studied a lot of documentation regarding file change notifications on Windows but this is the first time I see ERROR_NOTIFY_ENUM_DIR. When you google for it there's very few meaningful results for it, aside from the description, I don't see this error mentioned in ReadDirectoryChangesW docs, I used GetQueuedCompletionStatus() documentation describing the overflow. When I check ERR value by the time I find out about buffer overflow in my code (in wxIOCPThread::ReadEvents(), line 288) it's S_OK. So it's either: 1. this error code is not set when using an I/O completion port; 2. the error was cleared by another system call before I got to handle the overflow; 3. my code is wrong.

Unfortunately, I have more bad news. After a long time, I have downloaded the latest SVN version of wxWidgets, applied my fswatcher patch and recompiled to see if ERROR_NOTIFY_ENUM_DIR is set when I believe there was a file notify buffer overflow. To my unpleasant surprise, I noticed there are memory leaks in my fswatcher stress testing program, I have checked the (modified) fswatcher sample and those ugly leaks are there too, looking like the watches have not been deleted when fswatcher is destroyed. As I was sure there were no leaks when I submitted the patch, I checked the wxWidgets SVN change log. I noticed you switched to using STL containers in r67343, so I reverted to r67342, recompiled and the leaks are gone. So one more issue to take care of, I haven't tried if the leaks are there when using unmodified fswatcher implementation.

Sorry, I can't be of more help now.

comment:19 follow-up: Changed 3 years ago by vadz

  • Milestone 2.9.2 deleted
  • Owner vadz deleted
  • Status changed from accepted to new
  • Summary changed from Removing and adding back a path to wxFileSystemWatcher doesn't work to Better error checking reporting in wxFileSystemWatcher

I'm going to commit the changes fixing the issues 1, 2 and 3 soon but I'll keep the bug opened (but without urgent milestone) as we definitely need to do something about error reporting. Feedback from people using this class in real applications would be welcome.

comment:20 Changed 3 years ago by vadz

  • Patch unset
  • Status changed from new to confirmed

comment:21 Changed 3 years ago by VZ

(In [67691]) 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.

comment:22 Changed 3 years ago by VZ

(In [67692]) Don't watch directories recursively in wxMSW wxFileSystemWatcher.

MSW implementation of this class always watched the added entries recursively,
i.e. always behaved as if the entry to watch was added using AddTree().

Fix this simply by not asking ::ReadDirectoryChangesW() to watch the entire
subtree.

See #12847.

comment:23 Changed 3 years ago by VZ

(In [67693]) Implement watching directory correctly in MSW wxFileSystemWatcher.

The directories used to be always monitored recursively, even when this wasn't
requested, in wxMSW implementation. Change this but also implement efficient
support for monitoring the entire hierarchies using the native support for
this.

Also update the sample to allow monitoring directories recursively as well.

See #12847.

comment:24 Changed 3 years ago by VZ

(In [67694]) Fix wxFileSystemWatcher usage instructions.

Don't mention the virtual OnXXX() functions which were removed from the final
API.

Also mention AddTree() limitations on non-MSW platforms.

See #12847.

comment:25 in reply to: ↑ 19 Changed 3 years ago by PB

Replying to vadz:

I'm going to commit the changes fixing the issues 1, 2 and 3 soon but I'll keep the bug opened (but without urgent milestone) as we definitely need to do something about error reporting. Feedback from people using this class in real applications would be welcome.

I'm not even using wxWidgets anymore, so feel free to ignore my feedback.

On the other hand I kind of feel obliged to follow the development of this bug because I reported it, so here's my 2 cents.

I believe there's still a bit of work to be done when it comes to proper watch removal on MSW:

  1. wxFileSystemWatcher::RemoveAll() doesn't really remove any watch. wxFSWatcherImplMSW doesn't override wxFSWatcherImpl::RemoveAll() which just does "m_watches.clear()". Watches also have to be removed from wxIOCPService, it's the analogical issue as it was with Remove().
  1. You can't / shouldn't remove a watch added using efficient version of wxFileSystemWatcher::AddTree() by calling wxFileSystemWatcher::RemoveTree(). wxFileSystemWatcherBase::RemoveTree() always attempts to delete watches as if they were created one watch per each folder using the default inefficient algorithm in wxFileSystemWatcherBase::AddTree(). Of course, the thing is, even if you override RemoveTree() in wxMSWFileSystemWatcher, you won't know if the AddTree() was called without a filter and a single watch is being used for the whole folder tree of if there were many watches created.

BTW, IIRC "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()." as written in the SVN changelog is not exactly a correct description of how the removal bug manifests/ed. After a attempt at removing a watch, you got at most 1 file notification event, then wxFSWatcherImplMSW::SetUpWatch() failed to reissue the watch because it had been removed from its list - so no more notifies were sent. The watch remained in wxIOCPService list though, so it couldn't be added again. That's why the bug was not so easy to spot.

And even more nitpicking: wxFileSystemWatcher::AddTree() should not be tagged as pure virtual in the documentation. ;)


Regarding Issue 4. I hope that even with my broken English I managed to get across that when the buffer overflow happens, the application programmer not being notified about it is just a part of the bug. The other part is that in this case the watch is not reissued (ReadDirectoryChangesW is not called) and thus no more notifies will be produced, which may be even more serious issue.

comment:26 Changed 3 years ago by vadz

  • Priority changed from low to normal
  • Summary changed from Better error checking reporting in wxFileSystemWatcher to Fix Remove{All,Tree}() and better error checking reporting in wxFileSystemWatcher

Thanks for continuing to follow this up!

RemoveAll() is indeed still broken. I think the simplest way to implement it would be to really recreate wxIOCPService entirely, there doesn't seem to be much sense in keeping it.

RemoveTree() is also broken and we indeed need to remember how did we add the path in wxMSW implementation.

AddTree() documented as pure virtual: I can fix this :-)

Finally, I didn't realize (even though rereading what you wrote I do now) that the buffer overflow results in missing all the subsequent notifications so it's indeed even worse than I thought. I still think we ought to increase the buffer size to mitigate it for now but fixing it properly still must be done, of course.

comment:27 Changed 3 years ago by VZ

(In [67725]) Don't document wxFileSystemWatcher::AddTree() as pure virtual.

It isn't.

See #12847.

Changed 9 months ago by roberto

patch that creates an overflow warning type to wxFileSystemWatcherEvent

comment:28 Changed 9 months ago by roberto

  • Patch set

I have attached a patch that fixes the issue 4 mentioned above (ReadDirectoryChangesW() buffer overflow)

The patch does the following:

  1. creates a new property on wxFileSystemWatcherEvent class that holds the warning type (none, general, or overflow)
  1. in the MSW implementation, we look for the case when ReadDirectoryChangesW returns 0 for buffer size, as this indicates that the buffer overflowed. In this case, we generate a new warning event (if the watcher has the warning flag set) AND more importantly it will reissue that watch so that the watcher can be notified on new events.
  1. in the unix inotify implementation, when the warning event is generated we look at the native flags and if the native event is an overflow event we set the warning type property on the new event.

comment:29 Changed 7 months ago by vadz

Sorry, I still didn't have time to look at the patch in details myself but just wanted to note that #15426 describes a scenario in which the problem apparently still happens, even with the patch.

comment:30 Changed 6 months ago by vadz

Thanks, I've finally reviewed the patch and am going to commit it soon with some minor changes.

Thanks again Rob!

comment:31 Changed 6 months ago by VZ

(In [74950]) Generate events with specific wxFSW_WARNING_OVERFLOW type if applicable.

This allows the program to distinguish between some other, unspecified,
warnings and this one which can and does happen whenever too many changes
occur too quickly but which has a clearly defined work around: the state kept
inside the program just needs to be refreshed by rescanning the directory anew.

See #12847.

comment:32 Changed 6 months ago by vadz

  • Patch unset

Still leaving this open because of the other problems reported above, but clearing the patch checkbox as the patch was applied.

Note: See TracTickets for help on using tickets.