Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#14480 closed defect (fixed)

wxFileSystemWatcher: Various fixes for adding and removing watches

Reported by: dghart Owned by:
Priority: normal Milestone: 2.9.5
Component: GUI-all Version: stable-latest
Keywords: wxFileSystemWatcher Cc:
Blocked By: Blocking:
Patch: yes

Description

In wxFileSystemWatcherBase::AddTree the passed dir wasn't added to the watches, and only subdirs were added, not files.
In wxFileSystemWatcherBase::RemoveTree the passed dir wasn't removed, and RemoveTraverser::OnDir called RemoveTree() instead of Remove(); so files in subdirs got 'removed' multiple times, while dirs weren't at all.
These are fixed in AddTree.diff

wxFileSystemWatcherBase::Add currently asserts when presented a filepath that is already watched; there's an equivalent situation in Remove(). However this may arise validly if someone adds/removes individual items, and then does AddTree/RemoveTree on a parent dir. Such a situation can be reproduced in the fswatcher sample, which reports the error anyway.
AddRemove.diff removes those asserts.

The 'fswatcher' sample asserts on trying to remove a filepath added by AddTree(). This is mostly because AddTree() doesn't add the passed filepath; but there currently isn't a way to call RemoveTree() for trees, rather than just Remove(). fswatcher-sample.diff adds a prefix to the ListView strings which is later used to distinguish between trees and single watches, so they are removed appropriately. Tested in wxGTK and on WinXP.

fswatcher-test.diff adds a section to the FileSystemWatcherTestCase to test adding/removing items and trees. Tested in wxGTK.

In various places I've used wxFileName::DirName and similar to ensure that dirs have a terminal separator. Some of these are probably unnecessary, but it ensures that wxFileName doesn't mistake a dir for a file.

With the above patches plus #14465, wxFileSystemWatcher now works correctly for me in wxGTK; except for links, about which I'll open a thread on wx-dev.

Attachments (4)

AddRemove.diff download (2.3 KB) - added by dghart 6 years ago.
fswatcher-sample.diff download (2.0 KB) - added by dghart 6 years ago.
fswatcher-test.diff download (5.0 KB) - added by dghart 6 years ago.
AddTree.diff download (2.3 KB) - added by dghart 6 years ago.

Download all attachments as: .zip

Change History (16)

Changed 6 years ago by dghart

Changed 6 years ago by dghart

Changed 6 years ago by dghart

comment:1 Changed 6 years ago by dghart

  • Blocked By

(In #14465) Replying to vadz:

I wonder if we can rely on always getting IN_IGNORED. At least inotify_rm_watch(2) does seem to say that we should get it. And in this case we could fix the problem while keeping the code robust by maintaining a separate list of "just removed" watches and check that inevt.wd is present in this list if we don't find it in the main one. And, on receiving IN_IGNORED, remove it from this list as well to avoid it getting bigger and bigger with time.

Yes, it seems to be reliable. For the record, what happens is: a dir watch is removed; then its contents are removed, which generate IN_OPEN and IN_CLOSE_NOWRITE events in the dir, usually before its IN_IGNORE.

Your suggestion works well to solve this :) After the new patch and the #14480 ones wxFileSystemWatcher behaves properly here (modulo symlinks).

comment:2 Changed 6 years ago by dghart

  • Blocked By

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

This mostly looks good, thanks a lot! I have a question about the first (AddTree.diff) patch: wouldn't it better to call wxFileSystemWatcherBase::DoAdd() directly instead of the public Add() from AddTraverser::On{File,Dir}()? This should be more efficient as we avoid {File,Dir}Exists() tests in Add() for every path, which are completely unnecessary as we already know what they are, and also would avoid the need for brittle GetPathWithSep() use.

Concerning the removing of the asserts in the second patch (AddRemove.diff): this is probably the only thing to do but OTOH this does introduce an unfortunate situation when you might be watching, say, /var/log/foo, then added a watch for /var/log tree, removed it and ended up not watching /var/log/foo neither. IMO this is not ideal... Ideal would clearly be to keep a ref count for the number of times the given path is watched but until this is done I wonder if it wouldn't be better to keep the asserts. They shouldn't be a big problem in any realistic application (you usually know what you're watching and whether it's a subdirectory of another watched path) and might avoid some surprises.

Thanks a lot for the patches to the sample and the test!

comment:4 Changed 6 years ago by VZ

  • Blocked By

(In #14465) (In [72049]) Fix bogus asserts in Unix wxFileSystemWatcher for removed files.

Store the recently removed file descriptors and don't assert if we get an
event for one of them, this can happen and is normal unlike unexpected events
for completely unknown descriptors for which we still keep an assert.

Closes #14465.

Changed 6 years ago by dghart

comment:5 in reply to: ↑ 3 ; follow-up: Changed 6 years ago by dghart

  • Blocked By 14465 removed

Replying to vadz:

I have a question about the first (AddTree.diff) patch: wouldn't it better to call wxFileSystemWatcherBase::DoAdd() directly instead of the public Add() from AddTraverser::On{File,Dir}()? This should be more efficient as we avoid {File,Dir}Exists() tests in Add() for every path, which are completely unnecessary as we already know what they are, and also would avoid the need for brittle GetPathWithSep() use.

I agree about DoAdd(), and I've changed the patch. I don't see that we can avoid GetPathWithSep() though, as a wxString still needs to become a wxFileName. I've been bitten by this problem many times over the years, and become paranoid...

Concerning the removing of the asserts in the second patch (AddRemove.diff): this is probably the only thing to do but OTOH this does introduce an unfortunate situation when you might be watching, say, /var/log/foo, then added a watch for /var/log tree, removed it and ended up not watching /var/log/foo neither. IMO this is not ideal... Ideal would clearly be to keep a ref count for the number of times the given path is watched but until this is done I wonder if it wouldn't be better to keep the asserts. They shouldn't be a big problem in any realistic application (you usually know what you're watching and whether it's a subdirectory of another watched path) and might avoid some surprises.

I have no personal problem with keeping the asserts because I have no intention of using AddTree() anyway; in fact I can't think of a sensible use-case for it, at least in a file manager. I want to watch all visible dirs i.e. any that are expanded in the treectrl, and their immediate children. I certainly don't want to watch all children recursively; suppose someone selects $HOME or '/'.

comment:6 in reply to: ↑ 5 ; follow-up: Changed 6 years ago by vadz

Replying to dghart:

I agree about DoAdd(), and I've changed the patch.

Thanks!

I don't see that we can avoid GetPathWithSep() though

I thought we'd just call DoAdd(path, wxFSWPath_Dir), why not?

I have no personal problem with keeping the asserts

OK, let's keep them, i.e. not apply this patch for now then.

because I have no intention of using AddTree() anyway; in fact I can't think of a sensible use-case for it, at least in a file manager.

Probably not in a file manager. But a log monitoring application might want to monitor the entire /var/log, this is why I took this as an example.

comment:7 in reply to: ↑ 6 Changed 6 years ago by dghart

Replying to vadz:

I don't see that we can avoid GetPathWithSep() though

I thought we'd just call DoAdd(path, wxFSWPath_Dir), why not?

'path' has no terminal /, and the resulting wxFileName thinks it's a file. That works for DoAdd(), which doesn't check what it's passed.
However 'path' is stored in the watch, still without the terminal /. When later it's removed, Remove() compares it with the correctly-terminaled string, fails to find it and asserts.

Of course we could pass the wrong string in both places (I've not tested); but why not do it correctly? As I said, I've had been caught out so often in similar situations that I've become obsessional.

comment:8 Changed 6 years ago by vadz

I find it dangerous to rely on the presence of this slash. We probably ought to store the kind of the file (plain or directory) instead. But for now I'll just apply this patch, thanks.

comment:9 Changed 6 years ago by VZ

  • Resolution set to fixed
  • Status changed from new to closed

(In [72066]) Add the root and all the files in wxFileSystemWatcherBase::AddTree().

When watching a tree recursively, add the files and not only the directories.

Also, add -- and remove in RemoveTree() -- the root directory itself and not
only its children.

Closes #14480.

comment:10 Changed 6 years ago by VZ

(In [72067]) Properly use RemoveTree() in fswatcher sample.

We need to use RemoveTree() to remove watches for the paths added with
AddTree().

See #14480.

comment:11 Changed 6 years ago by VZ

(In [72068]) Test calling wxFileSystemWatcher::{Add,Remove}Tree().

Add a test for tree monitoring functions to the unit test.

See #14480.

comment:12 Changed 6 years ago by VZ

(In [72080]) Disable the recently added wxFileSystemWatcher unit case under Windows.

It currently fails there, so don't run it until this is fixed to let the tests
pass globally.

See #14480.

Note: See TracTickets for help on using tickets.