Proposed API Changes #64
Comments
|
Event may not the best name for an interface, especially since it's also the name of our outgoing channel. It may be better to think of a FileEvent as a notification, and therefore the interface as a Notifier. Other ideas? |
|
I like it, but what's the interface for? From what I know you can't send/recv an interface over a channel and it has to be a type (like a struct), which is why the channel currently sends/recvs Events. Is it now possible to send Interfaces over channels? Or are you planning a use for the interface I'm not seeing? |
|
The reason I am wanting to add an interface is so that I can make a fakeEvent for unit testing, eg. something along these lines: type fakeEvent struct {}
func (e *fakeEvent) IsCreate() bool { return true }
func (e *fakeEvent) IsDelete() bool { return false }
func (e *fakeEvent) IsModify() bool { return false }
func (e *fakeEvent) IsRename() bool { return false }
func (e *fakeEvent) FileName() string { return "filename.txt" }
func TestTriggerAllEventsFiltersNothing(t *testing.T) {
p := &pipeline{fsnFlags: FSN_ALL}
event := &fakeEvent{}
if forward := p.processEvent(event); !forward {
t.Fatal("Create event should be forwarded for FSN_ALL")
}
}I was experimenting with a chan of interface and it seemed like it would work. Though for now we would need to send concrete type over the chan for the Name field, and we could just keep this interface internal for testing purposes. |
|
One of my concerns with the WatchPath API above is if people add multiple overlapping recursive watches to the same Watcher. I haven't thought through all the strange edge cases that are made possible. |
|
Looking through the docs for Watchdog, it has an EventQueue that uses a thread-safe set in order to de-dup events. It has classes for different types of events, the only advantage I see being that file/directory move events have both the src & dest. It has a start() method once the Observer (equivalent to a NewWatcher) is configured. It looks like a pretty involved API compared to what we have here, but it may be exposing more internals than absolutely necessary. The Watchdog docs have a useful section on supported platforms and caveats. Listen uses a chainable API which is quite feasible to do in Go, though I experimented with it and found it unnecessarily complicates matters. Listen.to('dir/path/to/listen', 'dir/to/other_app')
.ignore(%r{^ignored/path/})
.filter(/\.rb$/)
.latency(0.5).startListen always uses recursive watches, but there are plans to add a (non)recursive option. It has an option to determine whether to use relative or absolute paths in the callbacks. It also supports pausing and resuming. For a more drastic change, I'm still trying to understand if and how @campoy's recommendation to "Avoid concurrency in your API" would apply. Tweet |
|
A number of the file system event APIs use callbacks in other languages. Ignoring Options for the moment, it might look something like this: type EventHandler func(Event, error)
func (w *Watcher) WatchPath(path string, handler EventHandler) {
}
func main() {
// using a callback
w := NewWatcher()
w.WatchPath("./", func(event Event, err error) {
if err != nil {
fmt.Println(err)
return
}
fmt.Println(event)
})
// using channels
evCh := make(chan Event)
errCh := make(chan error)
w.WatchPath("./", func(event Event, err error) {
if err != nil {
errCh <- err
} else {
evCh <- event
}
})
// do something with the channels...
}Events on the internal channel would just be sent to this callback function whenever an event happens. Pros/cons? (vs. present) |
|
Hi Nathan, The point of my slide "Avoid concurrency in your API" is actually: "Avoid concurrency in your API WHEN POSSIBLE" In your case, I'd try removing the Event and Error chans from the Watcher type and instead add methods to provide callbacks executed in case of errors - similarly to how the http package is organized. Then you could do something like http://play.golang.org/p/zNaOVHxQS6 Then you could add more fancy handlers so the the events can be handled more in detail, something like: func (w *Watcher) HandleEvent(filter func(ev *FileEvent) bool, handler func(ev *FileEvent)) |
|
@campoy Thanks. Using a API similar to the http package is an interesting idea, and your example actually looks pretty similar to Watchdog's Python API. Obviously there are considerations for breaking changes/deprecations, so I don't know if now is a good time to make such I change. I'll defer to @howeyc. :-) |
|
@howeyc Should FD_SET, FD_ISSET and FD_ZERO be part of the exported API, or should these be renamed sys_FD_SET, etc.? I don't see them being used, nor know what they do. |
|
FD_SET, FD_ISSET and FD_ZERO should not be exported. They were functions I added to do a Select on the watch descriptor before a blocking Read on Linux. The idea was to Select and then check "done" before Read but when I did that I got reports of events not triggering any more, so I removed it. As for the API, what about instead of callbacks, we make the channels internal to the library and add a blocking NextEvent() function to a Watcher. For example: func (w *Watcher) NextEvent() (event FileEvent, err error) {
select {
case event = <- w.internalEvent:
case err = <- w.Error:
}
return
}Also, maybe change the FileName() to FilePath(). |
|
Using an iterator could make for a very nice API. The calling code/example would likely use a go routine, would that make #7 impossible? I like the simplicity of your implementation. It seems like we could do this and easily keep around the Event channel for the 55 packages that import fsnotify, just marking it DEPRECATED(-) until a v1.0.0 release. My pull request uses Path() instead of FileName() and I ensured tests/code is in place to handle relative paths for the pipeline steps that I've implemented. Should FD_SET and friends be renamed sys_FD_SET or simply deleted in a nice clean commit that could be reverted later if desired? |
|
I agree that an iterator would be nice. And yes I believe it would make #7 For now, perhaps we should get rid of FD_SET and friends. On 22 September 2013 10:37, Nathan Youngman notifications@gitpro.ttaallkk.topwrote:
|
|
Okay. Might have to think about that. As far as the specific Options and which ones are per-Watch vs. for the entire Watcher, I think we may need to start implementing the adapter-specific throttling and recursion before it really becomes clear. I'm pretty out of my element in the low level guts of FSEvents, kqueue, etc (not to mention all these Mutexes and pointers flying around). Fyi, I started a conversation on deprecating APIs on the Go Nuts mailing list: https://groups.google.com/forum/#!topic/golang-nuts/MgG8JTVLs20 Hopefully nobody is using FD_SET and friends. They were only available on the Linux so they'd run into problems anyway. |
|
Wondering if I'm adding more than necessary. Maybe the The recursive directory option really needs to avoid hidden folders/files (such as .git), especially with kqueue where we are file handle limited, so I think the hidden option must stay. |
|
More discussion of the API over here: https://gist.github.com/howeyc/7156865 |
|
Rich Hickey's core.async talk provides some good reasoning for not using callbacks. So 👍 for an iterator interface? I need to try that out still, and see how it feels to use from a real app. I'm still wondering if the Options{} should be specified per Watcher or per WatchPath() as in #65 at present. It seems like it might be simpler with the former. Indeed, I'm not 100% certain on the value of multiple WatchPaths per Watcher. Finally, I'm starting to think more about testing for apps that use fsnotify. In particular, isolation testing (or unit tests) that don't need to write files and such like our end-to-end integration tests do. The interfaces we've been adding should help, but I'd like to get an app like Looper under test to be sure. |
|
|
I'm currently working on an internal event processing pipeline that will replace
purgeEvents(), calling a series of functions to handle recursive watches (#56), throttling (#62), along with basic filtering and logging. I would prefer to keep the pipeline itself internal, at least initially, but there are still some API changes.fsnotify.Event interface
In order to unit test pipeline steps, I would like to fake the event being passed in, which can be accomplished with an interface:
As an interface cannot contain a field, the pipeline expects a Path() method, rather than a Name field.
The Watcher API still provides a concrete
Event chan *FileEventthat contains a Name field in addition to Path(). Even so, I think it would be good to export the Event interface and to mark the Name field as deprecated.New Options
These options don't necessarily correspond one-to-one with the steps in the pipeline, but they do represent the configuration those steps need.
In order to specify these options, a new
WatchPathmethod is introduced. The options have reasonable defaults, mostly using zero-initialization, though ThrottleDuration and Triggers don't have zero value defaults.Watch & WatchFlags work as before, but are marked as deprecated. Of note, they continue to watch hidden files/directories (WatchHidden: true).
It may be more appropriate if some of these options were specified at a "global" level rather than per-Watch, but I would rather not change the NewWatcher() API. I suppose we could add some accessors to the Watcher instead?
A number of options don't really make sense when watching a single file. I wonder if anyone is using fsnotify for that purpose, and whether or not FSEvents (#54) supports single file watches. If so, I'm sure we can come up with a solution beyond "don't use these options in this case", perhaps a WatchFile() method?
Triggers type
When looking through the Go standard library, I've seen constants in ALL_CAPS and TitleCase. I'm not aware of an official reason to choose one over the other, but I find the following looks a little nicer on a page than
FSN_CREATE, etc.Outside of introducing a type for the constants, this is mostly an aesthetic change. The internal representation is exactly the same.
Comments
While working away on a first pull request, I would appreciate any early feedback on the naming and approach described here. Thanks!
The text was updated successfully, but these errors were encountered: