Notify about missed event on Windows, MacOs & Linux.

[ed-sat]

Hi. We use your library for monitoring changes in our repository on different OS.
And we noticed that in some situation Windows OS loses file events due to buffer overflowed. In this situation client should resync file tree manually. So I added listener notification to inform client about missing file actions.

For information: chromium code do it in the same way
https://source.chromium.org/chromium/chromium/src/+/main:content/browser/file_system_access/file_path_watcher/file_path_watcher_win.cc;l=485;drc=8d1bf30bd5eccfc602479995ba9e7d886c93d1b5;bpv=0;bpt=1

https://source.chromium.org/chromium/chromium/src/+/main:content/browser/file_system_access/file_path_watcher/file_path_watcher_win.cc;drc=8d1bf30bd5eccfc602479995ba9e7d886c93d1b5;bpv=0;bpt=1;l=563

[SpartanJ]

Hi @ed-sat!
Thanks for your collaboration.

And we noticed that in some situation Windows OS loses file events due to buffer overflowed.

Just FYI: you can handle the default buffer size used in efsw with the efsw::Options::WinBufferSize. WatcherOptions can be passed during the addWatch call:
fileWatcher.addWatch( yourPath, ul, true, { efsw::WatcherOption( efsw::Options::WinBufferSize, 256 * 1024 ) } ).
This is what people usually used when were limited by the default buffer size, which is limited intentionally to 63k due to what the documentation states:

namespace Options {
enum Option {
	/// For Windows, the default buffer size of 63*1024 bytes sometimes is not enough and
	/// file system events may be dropped. For that, using a different (bigger) buffer size
	/// can be defined here, but note that this does not work for network drives,
	/// because a buffer larger than 64K will fail the folder being watched, see
	/// http://msdn.microsoft.com/en-us/library/windows/desktop/aa365465(v=vs.85).aspx)
	WinBufferSize = 1,

As for the PR I'm a little undecided to merge it, it's an small API change and does not affect compilation on the C++ side, but still is a change, what I don't like is that the event is exclusive for a particular Windows event, I think I would like something more generic to communicate state changes otherwise will complicate the C bindings. Also, as a nit, any API/ABI change must also have its C bindings counterpart implemented and this change would require to modify the efsw_addwatch and efsw_addwatch_withoptions function calls parameters which in this case would represent an ABI change. Let me think about it a little bit further, but it's a totally reasonable change to fix your particular issue.

[ed-sat]

Just FYI: you can handle the default buffer size used in efsw with the efsw::Options::WinBufferSize. WatcherOptions can be passed during the addWatch call:
fileWatcher.addWatch( yourPath, ul, true, { efsw::WatcherOption( efsw::Options::WinBufferSize, 256 * 1024 ) } ).

I have inscreased buffer size to 4mb during some tests and experiments on Windows OS but sometimes we still lose file events.

I have checked how implemented watching file events in chromium and git and found that the better way is to have notification about losing file actions.

For chromium on Windows OS:
https://source.chromium.org/chromium/chromium/src/+/main:content/browser/file_system_access/file_path_watcher/file_path_watcher_win.cc;l=485;drc=8d1bf30bd5eccfc602479995ba9e7d886c93d1b5;bpv=0;bpt=1
https://source.chromium.org/chromium/chromium/src/+/main:content/browser/file_system_access/file_path_watcher/file_path_watcher_win.cc;drc=8d1bf30bd5eccfc602479995ba9e7d886c93d1b5;bpv=0;bpt=1;l=563

For Git on Windows OS:
https://github.com/git/git/blob/cb0ae672aeabefca9704477ea8018ac94f523970/compat/fsmonitor/fsm-listen-win32.c#L408-L410
https://github.com/git/git/blob/cb0ae672aeabefca9704477ea8018ac94f523970/compat/fsmonitor/fsm-listen-win32.c#L550-L572

Also we can notify about losing events on MacOS when we using FSEvent implementation like in chromium and git
by notifying on events with flags:
kFSEventStreamEventFlagUserDropped | kFSEventStreamEventFlagKernelDropped | kFSEventStreamEventFlagMustScanSubDirs

For Chromium on Mac OS:
https://source.chromium.org/chromium/chromium/src/+/main:third_party/devtools-frontend/src/extensions/cxx_debugging/third_party/llvm/src/clang/lib/DirectoryWatcher/mac/DirectoryWatcher-mac.cpp;l=97-99
https://source.chromium.org/chromium/chromium/src/+/main:third_party/devtools-frontend/src/extensions/cxx_debugging/third_party/llvm/src/clang/lib/DirectoryWatcher/mac/DirectoryWatcher-mac.cpp;l=117-120

For Git on Mac OS:
https://github.com/git/git/blob/cb0ae672aeabefca9704477ea8018ac94f523970/compat/fsmonitor/fsm-listen-darwin.c#L134-L141
https://github.com/git/git/blob/cb0ae672aeabefca9704477ea8018ac94f523970/compat/fsmonitor/fsm-listen-darwin.c#L234-L264

Also I think we should process inotify event with flag IN_Q_OVERFLOW on Linux at the same way as for Windows and MacOs (FSEvents).

So eventually new event in listener will work on Windows OS, MacOS and Linux.

Unfortunately I haven't seen C-bindings I will think how to extend C-bindings to support new API.

[SpartanJ]

So eventually new event in listener will work on Windows OS, MacOS and Linux.

That would be great, in that case I think it's justified if macOS and Linux handleMissedFileActions events are also implemented. Are you willing to implement it in this PR? Please let me know so I can decide how to approach it.

Unfortunately I haven't seen C-bindings I will think how to extend C-bindings to support new API.

If the above case is true, adding an extra parameter seems to be justified, allowing to pass nullptr to skip any handling.

This will be an ABI break but I might take advantage of the situation to change the major version and change a couple of extra things about the API/ABI.

[ed-sat]

That would be great, in that case I think it's justified if macOS and Linux handleMissedFileActions events are also implemented. Are you willing to implement it in this PR? Please let me know so I can decide how to approach it.
I will implement handling missing event on MacOs and Linux and update PR when it will be ready.

[SpartanJ]

@ed-sat I think you forgot to post the comment, you just quoted my comment.

[ed-sat]

I will implement handling missing event on MacOs and Linux and update PR when it will be ready.

Sorry I wrote my part with quoted your comment. I updated PR with notification on MacOS & Linux

[ed-sat]

Should I extend C-Bindings API for missing file actions? Or this part you will implement by yourself?

[SpartanJ]

Should I extend C-Bindings API for missing file actions? Or this part you will implement by yourself?

I will add it later, thanks for your collaboration!