New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add AddWith() to pass options, allow controlling Windows buffer size #521
Conversation
Rather than make this configurable, perhaps we should just be changing the defaults for Windows?? What is software built using I guess I have reservations... |
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
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:
I kind of liked Essentially, there are three related issues here:
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. |
i can speak a bit to the use-cases if it's helpful. there's typically two problems around the buffer size.
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). |
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.
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:
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.
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? |
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 ☁️ ☁️ ☁️ |
Friendly ping @nshalman; I'm kind of waiting for your feedback on this 😅 (I don't expect anyone else will reply...) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's do it. I think this will be okay as long as we are careful about what kinds of options we allow.
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(time.Second), // 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
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:
Some other options we might want to add:
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():
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