Opened 21 months ago

Closed 18 months ago

Last modified 18 months ago

#14544 closed defect (fixed)

wxFileSystemWatcher: More fixes

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

Description

This is (I hope) the last of a series of wxFileSystemWatcher patches. The patches should be applied in this order, and on top of those from #14488, #14490, #14542 and #14543. (If you prefer I could instead create a Grand Unified Patch...)

First a reversion. In an earlier patch I tried to fix wxFileSystemWatcher:AddTree by having it watch files as well as dirs; this was partly because RemoveTree() was trying to remove those watches.
This was a mistake. Doing so is 1) unnecessary: the watch on the parent dir can manage its contents perfectly well by itself; and 2) Add() doesn't do this, and the difference in behaviour caused problems in dealing with creation/deletion in a watched tree. RevertAddTreeFiles.diff reverts this, and also removes from RemoveTree() the attempt to remove the unwatched files.

Next a correction to the #14480/[72049] fix. If a watched file is deleted, the first event to arrive is likely to be IN_IGNORED. At that stage the wd won't be in m_staleDescriptors, so trying to remove it causes an assert. StaleDescriptors.diff fixes this.

#14488 tried to make passing a file-mask happen at AddTree() level. This no longer works now that files themselves aren't watched. Instead Filespec.diff devolves it down to wxFSWatcherImpl::ProcessNativeEvent, which is much more flexible. This also avoids the issue raised in #14488 about whether to watch non-matching dirs. Filespecs now work well in wxGTK, and afaict also in wxMSW.

The current code lets the user add and remove watches, but ignores the possibility that a watched dir may be added/removed by the filesystem itself. CreateDelete.diff fixes this.

Finally, SampleAndTest.diff makes the sample notice if the directory it's watching is deleted or renamed, notifies the user, and removes it from the wxListView. This prevents the sample asserting (at least in wxGTK) when it tries to remove it later. It also updates FileSystemWatcherTestCase to work with the new code. I don't know if this will now fail on wxMac; if so, #if defined(UNIX) will need to be changed.

This patch series make wxFileSystemWatcher work well in wxGTK, without adversely affecting wxMSW.

Attachments (5)

RevertAddTreeFiles.diff download (1.4 KB) - added by dghart 21 months ago.
StaleDescriptors.diff download (901 bytes) - added by dghart 21 months ago.
Filespec.diff download (6.7 KB) - added by dghart 21 months ago.
CreateDelete.diff download (4.3 KB) - added by dghart 21 months ago.
SampleAndTest.diff download (5.2 KB) - added by dghart 21 months ago.

Download all attachments as: .zip

Change History (35)

Changed 21 months ago by dghart

Changed 21 months ago by dghart

Changed 21 months ago by dghart

Changed 21 months ago by dghart

Changed 21 months ago by dghart

comment:1 follow-up: Changed 21 months ago by dghart

  • Blocked By

(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.

comment:2 in reply to: ↑ 1 Changed 21 months ago by PB

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 21 months ago by dghart

  • Blocked By

(In #14488) Replying to PB:

Vadim may have meant it generally: underlying native MSW API doesn't support watching individual files so there's still that... :(

I hope I've worked-around that in #14544 by doing the filtering in wxFSWatcherImpl::ProcessNativeEvent. However I have very little wxMSW knowledge and, if you're still using wxFileSystemWatcher, it would be great if you could test these patches :)

comment:4 Changed 21 months ago by vadz

  • Blocked By

(In #14542) The patch looks mostly good but, as usual, I don't like the use of bool parameters in public API, i.e. in FileExists() and DirExists() and would prefer to use some kind of flags.

In fact, we could make wxFileSystemObject_XXX public and allow calling Exists(wxFileSystemObject_File | wxFileSystemObject_Link) (perhaps with shorter names?) to check for the existence of a link to a file?

Boolean argument is, of course, quite all right for DontFollowSymlinks() itself. I could apply this part of the patch even now if you'd like.

comment:5 follow-up: Changed 21 months ago by 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.

comment:6 in reply to: ↑ 5 Changed 21 months ago by dghart

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:7 Changed 21 months ago by dghart

  • Blocked By

(In #14542) Replying to vadz:

The patch looks mostly good but, as usual, I don't like the use of bool parameters in public API

I know :) but I thought this might be an exception, as it's difficult to think of any other flag that might be needed in the future.

In fact, we could make wxFileSystemObject_XXX public and allow calling Exists(wxFileSystemObject_File | wxFileSystemObject_Link) (perhaps with shorter names?) to check for the existence of a link to a file?

I think I'd choose the first way, and I'll change the patch to do that; but let me know if you'd rather use wxFileSystemObject direct.

Boolean argument is, of course, quite all right for DontFollowSymlinks() itself. I could apply this part of the patch even now if you'd like.

Thanks, but it'll be easier for me to redo the whole patch.

comment:8 Changed 21 months ago by dghart

  • Blocked By

(In #14542) I've updated the patch. I had to make the enum public anyway; it's now called 'wxFSItem', which is less descriptive but certainly shorter.

comment:9 Changed 18 months ago by vadz

  • Blocked By

(In #14488) To reply to the original question, I actually think the only behaviour that would really make sense would be to watch all files with the given filespec (only) in all the subdirectories. In particular, applying the filespec check to the directory doesn't make much sense to me and I'd rather not watch any subdirectories at all for now, this is at least more predictable, so I'll modify the patch to do it like this but if you can implement the above logic, it would be much better.

Another question is about the error messages I now reliably get when running the unit test:

Test program for wxWidgets non-GUI features
build: 2.9.5 (wchar_t,Visual C++ 1500,wx containers,compatible with 2.8)
....17:11:27: Error: Unable to dequeue completion packet (error 5: access is denied.)
17:11:27: Error: Unable to dequeue completion packet (error 5: access is denied.)
..

OK (6)

They come from wxIOCPService::GetStatus() but I have no idea why do we get them :-( Would you by chance?

Thanks in advance!

comment:10 follow-up: Changed 18 months ago by vadz

  • Blocked By

(In #14488) Sorry, I realized that we do already watch the files in the subdirectories if they match the filespec because we iterate over all of them recursively, please disregard the first part of the previous comment.

comment:11 Changed 18 months ago by vadz

  • Blocked By

(In #14542) I'm going to apply the DontFollowLink() part of this patch but I'm still not totally happy about the changes to {File,Dir,}Exists() so I won't change the static variants of these functions -- just update the member ones to honour the setting of m_dontFollowLinks.

The main reason I'm unhappy about the static ones is pretty stupid but here it is: naming. I don't think wxFSItem is a good name (meaning not for the enum itself, as nobody cares about it, but as the prefix for its elements) but I can't for the life of me come with anything better. So instead of not doing anything, I've decided to apply the important part of the patch right now and add the other one later, when somebody comes up with a good idea about how to call these flags.

comment:12 Changed 18 months ago by vadz

  • Blocked By

(In #14543) Sorry, I should have noticed it immediately and not 3 months later, but I don't like the approach of this patch. The whole idea of adding wxFileName::DontFollowLink() was to avoid adding other "no follow" flags like this so instead of adding this wxFSW_EVENT_DONT_FOLLOW (which really doesn't fit into "events" enum at all) I'd prefer to just be able to do

wxFileName fn(wxFileName::DirName("..."));
fn.DontFollowLink();
fsw.Add(fn);

Granted, this stretches the meaning of this flag a little for AddTree() but it makes so much sense for Add() that I think it's forgiveable to extend it to AddTree() too.

What do you think?

comment:10 follow-up: Changed 18 months ago by vadz

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

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:11 Changed 18 months ago by VZ

  • Blocked By

(In #14488) (In [72678]) Improve handling of file spec in wxFileSystemWatcher::AddTree().

Fix watching too many files (i.e. even those not matching the provided spec)
and asserts when removing a recursive watch with a file spec in wxMSW.

Closes #14488.

comment:12 Changed 18 months ago by VZ

  • Blocked By

(In #14490) (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:13 Changed 18 months ago by VZ

  • Blocked By

(In #14542) (In [72680]) Add support for symlinks to wxFileName.

Allow to work with the symlinks themselves and not the file they reference by
calling the new wxFileName::DontFollowLink().

Update Unix wxDir implementation to not treat symlinks to directories as
directories, this ensures that we don't recurse into the directories outside
of the original parent accidentally.

Closes #14542.

comment:17 Changed 18 months ago by VZ

(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:18 Changed 18 months ago by VZ

(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:19 Changed 18 months ago by VZ

(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:20 Changed 18 months ago by VZ

  • Blocked By

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

See #14488, #14490, #14544.

comment:21 Changed 18 months ago by dghart

  • Blocked By

(In #14488) Replying to vadz:

Another question is about the error messages I now reliably get when running the unit test:

Test program for wxWidgets non-GUI features
build: 2.9.5 (wchar_t,Visual C++ 1500,wx containers,compatible with 2.8)
....17:11:27: Error: Unable to dequeue completion packet (error 5: access is denied.)
17:11:27: Error: Unable to dequeue completion packet (error 5: access is denied.)
..

OK (6)

They come from wxIOCPService::GetStatus() but I have no idea why do we get them :-( Would you by chance?

I have no MSW knowledge, but these errors were reported 18 months ago in #13294, and are (presumably) the ones mentioned in issue 4 of #12847 comments 13 and 18.

comment:22 in reply to: ↑ 10 Changed 18 months ago by dghart

  • Resolution fixed deleted
  • Status changed from closed to reopened

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:23 Changed 18 months ago by vadz

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

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

comment:24 Changed 18 months ago by VZ

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

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

See #14544.

comment:25 Changed 18 months ago by dghart

  • Blocked By

(In #14542) Replying to vadz:

The main reason I'm unhappy about the static ones is pretty stupid but here it is: naming. I don't think wxFSItem is a good name (meaning not for the enum itself, as nobody cares about it, but as the prefix for its elements) but I can't for the life of me come with anything better. So instead of not doing anything, I've decided to apply the important part of the patch right now and add the other one later, when somebody comes up with a good idea about how to call these flags.

How about wxFileSystemFlags (or wxFSFlags if you prefer brevity)? Or 'Mode'?

comment:26 Changed 18 months ago by vadz

  • Blocked By

(In #14542) Both are too generic IMHO, and we also already have wxFileSystemOpenFlags which is for (virtual) wxFileSystem and not (physical) wxFileName so it could be also a bit confusing. BTW, we also already have wxFileKind too.

So I guess it would need to be some new wxFileExistsFlags (or, actually, unnamed, because what do we need its name for?) enum with elements wxFILE_EXISTS_FILE (or REGULAR?), wxFILE_EXISTS_DIR, ... and wxFILE_EXISTS_NO_FOLLOW.

Any better suggestions?

comment:27 Changed 18 months ago by dghart

  • Blocked By

(In #14542) Replying to vadz:

Both are too generic IMHO, and we also already have wxFileSystemOpenFlags which is for (virtual) wxFileSystem and not (physical) wxFileName so it could be also a bit confusing. BTW, we also already have wxFileKind too.

OK

So I guess it would need to be some new wxFileExistsFlags (or, actually, unnamed, because what do we need its name for?) enum with elements wxFILE_EXISTS_FILE (or REGULAR?), wxFILE_EXISTS_DIR, ... and wxFILE_EXISTS_NO_FOLLOW.

I'm not sure I understand. If we don't bother with the static functions, do we need an extra flag in the enum at all? Even if so, is there any need to change the names from the anonymous wxFileSystemObject_Foo?

I'm perfectly happy to skip the statics. I added the flag to them for completeness, and because it self-documents the need to consider symlinks; but it's not that much extra work for the user to create a wxFileName object and call DontFollowLink().

Any better suggestions?

I don't dare make the alternative suggestion of reverting to a bool parameter ;)

comment:28 Changed 18 months ago by dghart

  • Blocked By

(In #14543) Replying to vadz:

Sorry, I should have noticed it immediately and not 3 months later, but I don't like the approach of this patch. The whole idea of adding wxFileName::DontFollowLink() was to avoid adding other "no follow" flags like this so instead of adding this wxFSW_EVENT_DONT_FOLLOW (which really doesn't fit into "events" enum at all)

The wxFSW_EVENT enum is currently unused except for wxFSW_EVENT_ALL so, if it's a naming issue, changing wouldn't be difficult (except for thinking of a better one).

I'd prefer to just be able to do

wxFileName fn(wxFileName::DirName("..."));
fn.DontFollowLink();
fsw.Add(fn);


Granted, this stretches the meaning of this flag a little for AddTree() but it makes so much sense for Add() that I think it's forgiveable to extend it to AddTree() too.


What do you think?

I'm happy with that. The main drawback is, as in #14542, the loss of the self-documenting nature of the extra flag.

Please let me know if you want me to rework the patch.

comment:29 Changed 18 months ago by vadz

  • Blocked By

(In #14543) I'm not sure why do you think this enum is unused, I think it is used and does allow to specify which events you're interested in, doesn't it?

I think we should either use the suggestion above or maybe replace (well, augment) Add() and AddTree() by some new method (Watch()?) which would take

  1. The path.
  2. The flags specifying the kind of watched to add (wxFSW_FLAGS_RECURSIVE, for the current AddTree(), to follow the links or not (wxFSW_FLAGS_NO_FOLLOW) and anything else we can think about.
  3. The event mask (wxFSW_EVENT_XXX combination).
  4. Pattern.

Actually I think that the latter approach might make more sense than the current Add()/AddTree() but it clearly would require more work too. So for now I'd be happy with the former, wxFileName::DontFollowLink()-using one.

TIA!

comment:30 Changed 18 months ago by vadz

  • Blocked By

(In #14542) Replying to dghart:

I'm not sure I understand. If we don't bother with the static functions, do we need an extra flag in the enum at all?

No, we don't, but I do agree that being able to use this functionality with static functions would be useful/convenient/less surprising.

Any better suggestions?

I don't dare make the alternative suggestion of reverting to a bool parameter ;)

Believe it or not, but I did consider it. However I'd also like to add a test for symlink existence as this can be useful and I'd really prefer to do it by adding flags to Exists() rather than by adding yet another LinkExists() so flags would be better here.

And for now the best I can see is to allow

if ( wxFileName::Exists(wxFILE_EXISTS_FILE | wxFILE_EXISTS_NO_FOLLOW) )
  ...

as I wrote above. OTOH it's not without its problems neither as this would mean that the non-static Exists() shouldn't take the same flags because wxFILE_EXISTS_NO_FOLLOW could conflict with m_dontFollowLinks.

So finally perhaps it's indeed better to do nothing for now and maybe add LinkExists() etc (FIFOExists()? `SocketExists()?) later if anybody asks for it.

Note: See TracTickets for help on using tickets.