Skip to content
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

Support filtering events, add unsupported events #629

Merged
merged 1 commit into from
May 1, 2024
Merged

Support filtering events, add unsupported events #629

merged 1 commit into from
May 1, 2024

Conversation

arp242
Copy link
Member

@arp242 arp242 commented Apr 28, 2024

This adds the ability to only listen for some event types. If you're only interested in Created events and you're getting a lot of Write events then you're just wasting CPU cycles

This also adds the ability listen on extra unportable event types; since this is so related I figured I might as well do both. Ideally we want to 1) make it very very obvious you're doing something unportable, and 2) make it reasonably easy "fallback" for platforms where this isn't supported.

Unportable events start with "Unportable", which should document their unportabilitiness. Also add a new Supports(Op) method, which should make adding fallback logic relatively painless.

For example, to use CloseWrite where supported, but falling back to Write when it's not:

var op fsnotify.Op
if w.Supports(fsnotify.UnportableCloseWrite) {
	op |= fsnotify.UnportableCloseWrite
} else {
	op |= fsnotify.Create | fsnotify.Write
}
w.AddWith("/tmp", fsnotify.WithOp(op))

And then you can deal with this in the write loop. There's a full example in cmd/fsnotify/closewrite.go

TODO: need to write tests for this, update all platforms.

Updates #7
Updates #519

@arp242 arp242 changed the title Support listening events, add unsupported events Support listing events, add unsupported events Apr 28, 2024
@nickajacks1
Copy link

Nice, Supports was a great idea because it lets the programmer explicitly decide what the fallback behavior is. While I'd personally prefer compile time checks, this explicit runtime check method is probably more approachable for people and more in the spirit of fsnotify. A message warning about Unsupported* events in the Readme could also help to make sure devs are aware of any potential misuse.

@arp242 arp242 changed the title Support listing events, add unsupported events Support filtering events, add unsupported events Apr 29, 2024
@arp242 arp242 force-pushed the events branch 2 times, most recently from 91388e3 to c5b5f4f Compare May 1, 2024 00:28
@arp242
Copy link
Member Author

arp242 commented May 1, 2024

While I'd personally prefer compile time checks, this explicit runtime check method is probably more approachable for people and more in the spirit of fsnotify.

In principle I agree, however looking at this in detail, one of my main issues with build tags is that authors will have to make two files like so:

has_closewrite.go:

//go:build linux || freebsd

var fsOp = fsnotify.UnportableCloseWrite

func procEvent(e fsnotify.Event) {
    if e.Has(fsnotify.UnportableCloseWrite) {
        fmt.Println(e)
    }
}

no_closewrite.go

//go:build !linux && !freebsd

var fsOp = fsnotify.Create | fsnotify.Write

func procEvent(e fsnotify.Event) {
    // Fallback with timers as per ./cmd/fsnotify/dedup.go
}

And this wouldn't be a huge deal, but those build tags aren't static, and platforms can add support for things like CloseWrite in the future, or fsnotify can add support for things that we didn't have before.

This is not hypothetical: NetBSD 10 (released last month) adds support for CloseWrite and a few other events. Maybe OpenBSD will add it next month or next year? Entirely possible. When/if we add support for fanotify, fsevents, polling, the feature sets for those backends will be different (but the GOOS build tag won't be).

So a 1:1 mapping of operating system to supported features is neither static nor perfect.

And it's not an uncommon situation for someone to be developing on, say, macOS, having a CI for Windows and Linux, and BSD systems will more or less "just work" automatically. By focusing on platforms (go:build linux) rather than features (w.Supports(fsnotify.UnportableCloseWrite)) things will probably break more on lesser-used systems, or use sub-optimal solutions on those systems.

All things considered, runtime checks are probably the right choice here, and prefixing the name with UnportableEvent should give enough of a clue that it's unportable, and people will have to make a conscious decision about this.


The main downside of Watcher.Supports is that it's tied to the Watcher. When I added AddWith() to pass options in #521, I imagined it could be used to select the backend, among other things; e.g.

w.AddWith("/path", fsnotify.WithFanotify())
w.AddWith("/path", fsnotify.WithPoll())

But that won't jibe with Watcher.Supports() as different paths can have different backends. I'm okay with just telling people to use different Watcher instances for different backends, as that's probably quite a rare use case and the prototype I did for this was quite complex. But we do need to be able to override the default backend selection. Having a method for that is probably okay:

func (w *Watcher) Backend(b Backend) error {
    if w.startedWatching {
        return errors.New("already started")
    }
    w.backend = b
    return nil
}

Adding options to NewWatcher() was discussed in #550 when NewBufferedWatcher() was added.

@arp242 arp242 force-pushed the events branch 7 times, most recently from 9b2980e to 2793221 Compare May 1, 2024 04:58
This adds the ability to only listen for some event types. If you're
only interested in Created events and you're getting a lot of Write
events then you're just wasting CPU cycles

This also adds the ability listen on extra unportable event types; since
this is so related I figured I might as well do both. Ideally we want to
1) make it very very obvious you're doing something unportable, and 2)
make it reasonably easy "fallback" for platforms where this isn't
supported.

Unportable events start with "Unportable", which should document their
unportabilitiness. Also add a new Supports(Op) method, which should make
adding fallback logic relatively painless.

For example, to use CloseWrite where supported, but falling back to
Write when it's not:

	var op fsnotify.Op
	if w.Supports(fsnotify.UnportableCloseWrite) {
		op |= fsnotify.UnportableCloseWrite
	} else {
		op |= fsnotify.Create | fsnotify.Write
	}
	w.AddWith("/tmp", fsnotify.WithEvents(op))

And then you can deal with this in the write loop. There's a full
example in cmd/fsnotify/closewrite.go

All of this is unexported for now, until support for other platforms has
been added.

Updates 7
Updates 519
@arp242 arp242 merged commit 08e056d into main May 1, 2024
18 checks passed
@arp242 arp242 deleted the events branch May 1, 2024 05:21
@arp242
Copy link
Member Author

arp242 commented May 1, 2024

Alright, I think the basic API and implementation for inotify should be good. I unexported all the new methods/types and merged to main. After the other platforms have been implemented we can export things and offer a public API.

Merging it now means we don't have to keep a bunch of long-lived branches around but can implement things a bit more piece-meal.

@arp242 arp242 mentioned this pull request May 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants