Add AddWith() to pass options, allow controlling Windows buffer size

[arp242]

This is similar to Add(), except that you can pass options. Ideally this should just be Add(), but that's not a compatible change, so we're stuck with this until we do a v2.

There are quite a few enhancements that depend on some way to pass options; as an example I added WithBufferSize() to control the buffer size for (see #72) for the Windows backend, because that one is fairly trivial to implement:

w, err := fsnotify.NewWatcher()
err = w.AddWith("/path", fsnotify.WithBufferSize(65536*4))

Some other options we might want to add:

err = w.AddWith("/path",
	fsnotify.WithEvents(fsnotify.Open | fsnotify.Close),  // Filter events
	fsnotify.WithPoll(10*time.Duration),                  // Use poll watcher
	fsnotify.WithFanotify(),                              // Prefer fanotify on Linux
	fsnotify.WithFollowLinks(true),                       // Control symlink follow behaviour
	fsnotify.WithDebounce(100*time.Milliseconds),         // Automatically debounce duplicate events
	fsnotify.WithRetry(1*time.Second, 1*time.Minute),     // Retry every second if the path disappears for a minute
)

These are just some ideas, nothing fixed here yet. Some of these options are likely to change once I get around to actually working on it.

This uses "functional options" so we can add more later. Options are passed to Add() rather than the Watcher itself, so the behaviour can be modified for every watch, rather than being global. This way you can do things like watch /nfs-drive with a poll backend, and use the regular OS backend for ~/dir, without having to create two watchers.

This upgrades fairly nicely to v2 where we rename AddWith() to Add():

err = w.Add("/path",
	fsnotify.WithBufferSize(65536*4),
	fsnotify.WithEvents(fsnotify.Open | fsnotify.Close))

Folks will just have to s/fsnotify.AddWith/fsnotify.Add/, without having to change all the option names. Plus having a consistent prefix autocompletes nicely in editors.

Fixes #72
Fixes #383

[nshalman]

Rather than make this configurable, perhaps we should just be changing the defaults for Windows??
It feels odd for an API like fsnotify to have options that only get used on some backends.
Consumers of fsnotify are ideally writing software that is ambivalent about which backend is being used at any given time.

What is software built using fsnotify.WithFanotify() supposed to do with that option when run on Windows? etc.

I guess I have reservations...

[arp242]

Rather than make this configurable, perhaps we should just be changing the defaults for Windows? It feels odd for an API like fsnotify to have options that only get used on some backends.

I increased the default from 4K to 64K some time ago; I don't know if 64K is always enough, or if there's a value that's always enough (a reasonable value anyway; pretty sure 100M will be always enough, but that's not reasonable). I looked a bit at some other solutions, and most have some sort of setting to control the buffer size.

Maybe @nicks or @milas has some context on this? I adapted it from their fork, including the 64K default I changed a few months back. I'm not that familiar with Windows.

---

About options in general:

I think it will be very hard to avoid having options that are only used by some backends. For example once we add the polling watcher we need to have a way to control the polling interval – there's no way we can choose one value that will work for every scenario – but that will do nothing on other backends.

The alternative is to create a new PollWatcher type that people can use, but that's even uglier IMO, and would also prevent adding features like automatic fallbacks if we detect an NFS filesystem. I'm not sure if there are any good alternatives where we still have one fsnotify.Watcher type.

What is software built using fsnotify.WithFanotify() supposed to do with that option when run on Windows?

My idea was that it would be ignored on Windows or when fanotify isn't available (Linux <5.9), so it's a "backend hint" rather than "set backend to this". I hadn't thought that much about it; although it's important we choose an API that will allow choosing the backend. Some other options might be:

// Use these backends if available; maybe WithBackendHint() is a better name?
WithBackend(fsnotify.WithFanotify)
WithBackend(fsnotify.WithPoll(time.Second))

// Choose the first one that's available
WithBackend(fsnotify.Fanotify, fsnotify.Inotify, fsnotify.Windows, fsnotify.Kqueue, fsnotify.FEN, fsnotify.Poll)

// Set for Linux
WithBackend(fsnotify.Linux, fsnotify.Fanotify, fsnotify.Inotify, fsnotify.Poll)
WithBackendLinux(fsnotify.Fanotify, fsnotify.Inotify, fsnotify.Poll)

I kind of liked fsnotify.WithFanotify() because it's fairly short, avoids users having to list all the backends, and doesn't require exporting a whole bunch of stuff. Realistically, there will only ever be a few "alternative" backends at the most (FSEvents on macOS, USN on Windows, Fanotify on Linux, and Poll as a generic polling fallback).

---

Essentially, there are three related issues here:

1. How do we pass options (in general)?
2. How do we select a backend?
3. How do we deal with options that only apply to one backend?

I spent a long time prototyping various solutions for selecting a backend some months back, and this is the only solution that I could come up with that I liked, but I'm very open to alternatives.

[nicks]

i can speak a bit to the use-cases if it's helpful. there's typically two problems around the buffer size.

• the user wants to watch an output directory for build artifacts and take some action when there are new artifacts. occasionally, there's a particularly chatty build system that splats a ton of artifacts at once (think: gradle generating a bunch of icons). in this case, the user will always want to increase the buffer size. The buffer size will be proportional to the size of the repo and the number of build artifacts.

• the user wants to watch a vendor directory of inputs, and re-sync when they change. occasionally, there's a big change (think: they do a major react version upgrade, or do a git checkout). In this case, it's an easy short-term fix to increase the size of the buffer. But long-term it would be better to have fsnotify emit some sort of OverflowOp - "hey, too many events overflowed the queue. You should re-sync to get all the changes up to timestamp X, where X is when we can guarantee the buffer stopped overflowing"

i guess i could imagine fsnotify offering some more abstract scalar factor to increase the buffer. Like instead of saying "I need a 64K buffer", I could say "I need a buffer 10x as big as the default" or "I need a buffer that handles bursts of 1000 file changes", and fsnotify could estimate how that works out in the underlying API. But I think better APIs for handling the overflow case would help too.

(but agreed that setting backend-specific options is easier on the implementation side, and there's definitely value in having an implementation that's easier to reason about).

[arp242]

Thanks @nicks; do you have any information on what a "typical" buffer size people want in those cases? 128K? 1024K? Even higher? Would be useful to document roughly what kind of value people should think about instead of trial-and-erroring it. I only have a very slow QEMU VM with Windows which isn't representative of real-world cases, so I can't really test this myself.

Like instead of saying "I need a 64K buffer", I could say "I need a buffer 10x as big as the default" or "I need a buffer that handles bursts of 1000 file changes", and fsnotify could estimate how that works out in the underlying API. But I think better APIs for handling the overflow case would help too.

The pathname is in the buffer, so that would be a rough guess. Plus, setting the buffer size larger than 64K doesn't always work; from the Windows docs:

ReadDirectoryChangesW fails with ERROR_INVALID_PARAMETER when the buffer length is greater than 64 KB and the application is monitoring a directory over the network. This is due to a packet size limitation with the underlying file sharing protocols.

So the actual size in bytes does matter at least in some cases. This also means we can't change the default to something larger, so "use a value that works for everyone" doesn't seem like an option.

I looked a bit at automatically increasing the buffer size when it's too small, but it seemed fairly complex to do correctly which ensures no events are lost (better issue an error than drop events), and this is made even more complex by the fact that you don't want this automatic resizing behaviour on networked filesystems.

In the 7.5 years the issue has been open no one offered a patch for this beyond the option in your fork and I'm not going to work on it any time soon either, so I'm not expecting a better fix any time soon. Unless someone offers to write a better solution, something along these lines seems like the best option for now.

"hey, too many events overflowed the queue. You should re-sync to get all the changes up to timestamp X, where X is when we can guarantee the buffer stopped overflowing"

I changed the error to a ErrEventOverflow sentinel error in this PR. Can't really add the time since ErrEventOverflow already exists (used by inotify) without breaking compatibility, so then we'd have two errors with a very similar meaning which is a bit meh. In most cases, I think the time from when you get this error should be "good enough", I think?

[nicks]

Tilt defaults it to 64K. If you overflow, we tell you "hey, you overflowed, set this env variable to X to increase the buffer", and suggest a bigger value for X. We don't track what values people use.

re: "I'm not going to work on it any time soon either" - 😂 ya, that's 100% reasonable, my comment was meant to be blue-sky dreaming ☁️ ☁️ ☁️

[arp242]

Friendly ping @nshalman; I'm kind of waiting for your feedback on this 😅 (I don't expect anyone else will reply...)

[nshalman]

Let's do it. I think this will be okay as long as we are careful about what kinds of options we allow.