Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#14490 closed defect (fixed)

wxFileSystemWatcher: Use reference counting for wxFSWatchInfo

Reported by: dghart Owned by:
Priority: normal Milestone: 2.9.5
Component: base Version: stable-latest
Keywords: wxFileSystemWatcher wxFSWatchInfo Cc:
Blocked By: Blocking: #14544, #14544, #14544, #14544, #14544, #14544, #14544, #14544, #14544, #14544, #14544
Patch: yes

Description

As discussed in #14480 and on wx-dev, wxFileSystemWatcher (at least in wxGTK) can't currently cope with the not-impossible situation of someone creating a watch on a directory, and later doing AddTree() on a parent dir. The patch implements reference counting for wxFSWatchInfo to make this possible, and tests it in FileSystemWatcherTestCase. (It was created on top of the #14488 patches.)

Tested successfully in FileSystemWatcherTestCase and in the 'fswatcher' sample on wxGTK and wxMSW.

Attachments (1)

refcount.diff download (7.2 KB) - added by dghart 2 years ago.

Download all attachments as: .zip

Change History (14)

Changed 2 years ago by dghart

comment:1 Changed 2 years ago by dghart

  • Blocking 14544 added

comment:2 Changed 2 years ago by PB

  • Blocking

(In #14544) Replying to dghart:

(In #14488) Replying to vadz:

I wonder if we shouldn't just remove the filespec argument. If it's really going to simplify the implementation a lot, I'd be ready to sacrifice it. After all, you always can easily filter out the files you're not interested in in your event handler and usually you'd want to watch all files anyhow, wouldn't you? If we want to keep support for this feature, we'd need to add individual watches for all files matching it under the given tree. And possibly also internal watches for all directories that would be handled to add watches for any newly created files. This doesn't seem easy to do...

Fortunately the latest patch, #14544, changes how a filespec is handled and solves these issues.

Vadim may have meant it generally: underlying native MSW API doesn't support watching individual files so there's still that... :( I believe that the documentation should mention that watching individual files and using filter is not supported on MSW.

comment:3 Changed 2 years ago by vadz

  • Blocking

(In #14544) I'll try to look at this series soon although to be honest I'm a bit intimidated. It might be better to redo a series of patches (0001-Something, 0002-SomethingElse, ...) if parts of the later patches change something done by the earlier ones as this should make reviewing them simpler.

I.e. perhaps it would be more convenient for both of us if you created a new branch, cherry pick all your commits there and used git rebase -i to reorganize them in the optimal way and then just used git format-patch?

If not, I'll try to look at this stuff starting from #14488 on Thursday.

comment:4 Changed 2 years ago by dghart

  • Blocking

(In #14544) Replying to vadz:

I'll try to look at this series soon although to be honest I'm a bit intimidated. It might be better to redo a series of patches (0001-Something, 0002-SomethingElse, ...) if parts of the later patches change something done by the earlier ones as this should make reviewing them simpler.

I.e. perhaps it would be more convenient for both of us if you created a new branch, cherry pick all your commits there and used git rebase -i to reorganize them in the optimal way and then just used git format-patch?

If not, I'll try to look at this stuff starting from #14488 on Thursday.

I tried to keep the patches clean and simple. With the exception of the unit-test, there are only minor 'internal' reversions in the series; by minor I mean just a couple of lines. So I think you'll find it easier that you expect.

If not, I'll redo them differently.

comment:5 Changed 2 years ago by vadz

  • Blocking

(In #14544) Thanks, I've applied everything here, I think, but I had to do it by combining some patches to avoid breaking unit tests.

The only change I intentionally did (as opposed to any number of bugs I could have introduced unintentionally...) was to change the filespec check to use the full file name instead of just its extension, I don't see any reason why we shouldn't allow watching for e.g. "syslog*".

Please let me know if I broke anything else and thanks again for all your work on this!

comment:6 Changed 2 years ago by VZ

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

(In [72679]) Make wxFileSystemWatcher watch entries reference-counted.

This helps to avoid problems that arise from watching the same physical file
system path multiple times, which could happen when adding a watch for a path
already watched because of a recursive watch on a parent directory, for
example.

Closes #14490.

comment:7 Changed 2 years ago by VZ

  • Blocking

(In #14544) (In [72681]) Check for filespec when generating events in wxFileSystemWatcher.

Instead of setting watches on individual files when a non-empty filespec is
given, always watch all the files but just ignore the events from the ones not
matching the filespec. This makes the code simpler and fixes several bugs.

See #14544.

comment:8 Changed 2 years ago by VZ

  • Blocking

(In #14544) (In [72682]) Fix bug in Unix wxFileSystemWatcher implementation when watch is deleted.

Don't assert when removing a watch descriptor from the stale descriptors list.

See #14544.

comment:9 Changed 2 years ago by VZ

  • Blocking

(In #14544) (In [72683]) Handle deletion of watched directories in wxFileSystemWatcher sample.

Don't assert when trying to stop watching a directory that doesn't exist any
more later.

See #14544.

comment:10 Changed 2 years ago by VZ

(In [72684]) Mention David Hart bug fixes in wxFileSystemWatcher.

See #14488, #14490, #14544.

comment:11 Changed 2 years ago by dghart

  • Blocking

(In #14544) Replying to vadz:

Thanks, I've applied everything here, I think, but I had to do it by combining some patches to avoid breaking unit tests.

Sorry to reopen this, but unless I'm missing something (which is quite possible; the complex series of patches confuses me too) I don't think you applied CreateDelete.diff

comment:12 Changed 2 years ago by vadz

  • Blocking

(In #14544) Oops, sorry about this and thanks for noticing it. Hopefully nothing else got forgotten, so re-closing it, thanks again.

comment:13 Changed 2 years ago by VZ

  • Blocking

(In #14544) (In [72689]) Improve inotify()-based wxFileSystemWatcher to handle creation/deletion.

Handle creation and deletion of directories under the watched path better.

See #14544.

Note: See TracTickets for help on using tickets.