Issue 48310043: code review 48310043: os/fsnotify: API sketch
Descriptionos/fsnotify: API sketch
This CL is the first draft of the API for the new os/fsnotify package.
It is based on http://godoc.org/github.com/howeyc/fsnotify.
Once the API is settled, we'll implement it everywhere.
Patch Set 1 #Patch Set 2 : diff -r 1b7c5daffdff https://code.google.com/p/go/ #
Total comments: 4
Patch Set 3 : diff -r 1b7c5daffdff https://code.google.com/p/go/ #
Total comments: 23
MessagesTotal messages: 27
Hello golang-codereviews@googlegroups.com, I'd like you to review this change to https://code.google.com/p/go/
Sign in to reply to this message.
On 2014/01/07 01:24:32, rsc wrote: > Hello mailto:golang-codereviews@googlegroups.com, (cc: mailto:cespare@gmail.com), > > I'd like you to review this change to > https://code.google.com/p/go/
Sign in to reply to this message.
https://codereview.appspot.com/48310043/diff/40001/src/pkg/os/fsnotify/event.go File src/pkg/os/fsnotify/event.go (right): https://codereview.appspot.com/48310043/diff/40001/src/pkg/os/fsnotify/event.... src/pkg/os/fsnotify/event.go:44: Event <-chan Event It feels odd to name a channel for what comes over that channel. Perhaps this could be at least called Events so that when you write a range loop over this it reads a bit more naturally. Could you also duplicate the bit about this channel being closed when the Close method is called up here?
Sign in to reply to this message.
https://codereview.appspot.com/48310043/diff/20001/src/pkg/os/fsnotify/event.go File src/pkg/os/fsnotify/event.go (right): https://codereview.appspot.com/48310043/diff/20001/src/pkg/os/fsnotify/event.... src/pkg/os/fsnotify/event.go:11: // TODO: What is the Windows equivalent? Where can we find what it's capable of? FindFirstChangeNotification or ReadDirectoryChangesW? http://msdn.microsoft.com/en-us/library/aa364417%28VS.85%29.aspx http://msdn.microsoft.com/en-us/library/aa365465(v=vs.85).aspx https://codereview.appspot.com/48310043/diff/20001/src/pkg/os/fsnotify/event.... src/pkg/os/fsnotify/event.go:24: Write do we want to add optional support for non-universally supported events like Open, and Read? https://codereview.appspot.com/48310043/diff/20001/src/pkg/os/fsnotify/event.... src/pkg/os/fsnotify/event.go:39: File string Sys interface{} for system specific information? https://codereview.appspot.com/48310043/diff/20001/src/pkg/os/fsnotify/event.... src/pkg/os/fsnotify/event.go:53: // the behind-the-scenes work to translate to the fds wanted by the kernels. I think the FindFirstChangeNotification API only work on filenames, and ReadDirectoryChangesW only work on directory handles. so using a filename instead a uintptr fd is better.
Sign in to reply to this message.
On 2014/01/07 01:43:35, minux wrote: > https://codereview.appspot.com/48310043/diff/20001/src/pkg/os/fsnotify/event.go > File src/pkg/os/fsnotify/event.go (right): > > https://codereview.appspot.com/48310043/diff/20001/src/pkg/os/fsnotify/event.... > src/pkg/os/fsnotify/event.go:11: // TODO: What is the Windows equivalent? Where > can we find what it's capable of? > FindFirstChangeNotification or ReadDirectoryChangesW? > http://msdn.microsoft.com/en-us/library/aa364417%2528VS.85%2529.aspx > http://msdn.microsoft.com/en-us/library/aa365465%28v=vs.85%29.aspx > > https://codereview.appspot.com/48310043/diff/20001/src/pkg/os/fsnotify/event.... > src/pkg/os/fsnotify/event.go:24: Write > do we want to add optional support for non-universally supported events like > Open, and Read? > > https://codereview.appspot.com/48310043/diff/20001/src/pkg/os/fsnotify/event.... > src/pkg/os/fsnotify/event.go:39: File string > Sys interface{} > > for system specific information? > > https://codereview.appspot.com/48310043/diff/20001/src/pkg/os/fsnotify/event.... > src/pkg/os/fsnotify/event.go:53: // the behind-the-scenes work to translate to > the fds wanted by the kernels. > I think the FindFirstChangeNotification API only work on filenames, > and ReadDirectoryChangesW only work on directory handles. > so using a filename instead a uintptr fd is better.
Sign in to reply to this message.
https://codereview.appspot.com/48310043/diff/40001/src/pkg/os/fsnotify/event.go File src/pkg/os/fsnotify/event.go (right): https://codereview.appspot.com/48310043/diff/40001/src/pkg/os/fsnotify/event.... src/pkg/os/fsnotify/event.go:11: // TODO: What is the Windows equivalent? Where can we find what it's capable of? implemented in https://code.google.com/p/go/source/browse/fsnotify/fsnotify_windows.go?repo=exp
Sign in to reply to this message.
https://codereview.appspot.com/48310043/diff/40001/src/pkg/os/fsnotify/event.go File src/pkg/os/fsnotify/event.go (right): https://codereview.appspot.com/48310043/diff/40001/src/pkg/os/fsnotify/event.... src/pkg/os/fsnotify/event.go:39: File string What about adding Extra string For Op specific data, mainly to deal with the rename case. https://codereview.appspot.com/48310043/diff/40001/src/pkg/os/fsnotify/event.... src/pkg/os/fsnotify/event.go:48: func NewWatcher() *Watcher kqueue and inotify use a file descriptor, so there is a potential that creating a Watcher may fail. Also, for solaris and plan9 there may be no suitable facility, so they need to be able to return EPLAN9 or similar.
Sign in to reply to this message.
https://codereview.appspot.com/48310043/diff/40001/src/pkg/os/fsnotify/event.go File src/pkg/os/fsnotify/event.go (right): https://codereview.appspot.com/48310043/diff/40001/src/pkg/os/fsnotify/event.... src/pkg/os/fsnotify/event.go:11: // TODO: What is the Windows equivalent? Where can we find what it's capable of? I think the key function is ReadDirectoryChangesW http://msdn.microsoft.com/en-us/library/windows/desktop/aa365465(v=vs.85).aspx https://codereview.appspot.com/48310043/diff/40001/src/pkg/os/fsnotify/event.... src/pkg/os/fsnotify/event.go:14: type Op uint64 Why not uint32, in case it ever matters? Is it conceivable that we will ever have more than 32 different kinds of events? https://codereview.appspot.com/48310043/diff/40001/src/pkg/os/fsnotify/event.... src/pkg/os/fsnotify/event.go:39: File string Doc needs to clarify what the value of File is when a change occurs in a directory being watched.
Sign in to reply to this message.
Looking at the API so far, it is oriented around files. Inotify is able to do a watch on an entire directory rather than just a file. The current kqueue adapter has a function sendDirectoryChangeEvents "to have the BSD version of fsnotify match linux fsnotify which provides a create event for files created in a watched directory." Extending upon that further, Windows has a bWatchSubtree option to do a recursive watch (not yet implemented in fsnotify). FSEvents on OS X operates at the directory level and I'm told it ONLY does recursive watches. We've seen requests for FSEvents, both to watch large trees where kqueue will run out of file handles, and because the daemon used for Spotlight search (mds) triggers many additional events when using kqueue (I suspect FSEvents will behave better based on file watching implementation in other languages). My hope is that fsnotify will continue to smooth out the differences between these adapters, as well as File Event Notifications when Solaris is officially supported. For the work I've been doing on fsnotify (on GitHub atm), I've been coming from a high level, implementing functionality that myself and others have implemented on top of fsnotify again and again. I don't know that all that functionality belongs in os/fsnotify vs. an auxiliary third party library. It's nice to see all the back-and-forth here, and nice to not be constrained by the existing API (eg. Events channel rather than Event). These are some API ideas I was tossing around 4 months ago, but I'm certain os/fsnotify can be much better. https://github.com/howeyc/fsnotify/issues/64
Sign in to reply to this message.
On Mon, Jan 6, 2014 at 11:42 PM, <nj@nathany.com> wrote: > Looking at the API so far, it is oriented around files. Inotify is able > to do a watch on an entire directory rather than just a file. 56 // If file names a directory, operations on files in that directory 57 // (but not in subdirectories) will also be reported. The recursive vs not recursive issue is interesting. What is natively supported in the various systems? Should OS X be using FSEvents instead of kqueue? If so, how? Russ
Sign in to reply to this message.
I too have found myself implementing recursive watches (and also rate limiting/throttling) every time I use fsnotify. There are some tricky aspects and you don't get the benefit of the native APIs various platforms provide. It makes sense if os/fsnotify is a low-level API that only provides the minimum common functionality between linux and bsd/darwin (and windows?). It would be great, though, should it turn out that these features need to be implemented in a cross-platform way for use in the go tool (which iirc is a large motivator for this os/fsnotify work), if they could at least be extracted to a go.tools package or elsewhere for easy usage. On Mon, Jan 6, 2014 at 8:42 PM, <nj@nathany.com> wrote: > Looking at the API so far, it is oriented around files. Inotify is able > to do a watch on an entire directory rather than just a file. The > current kqueue adapter has a function sendDirectoryChangeEvents "to have > the BSD version of fsnotify match linux fsnotify which provides a create > event for files created in a watched directory." > > Extending upon that further, Windows has a bWatchSubtree option to do a > recursive watch (not yet implemented in fsnotify). FSEvents on OS X > operates at the directory level and I'm told it ONLY does recursive > watches. We've seen requests for FSEvents, both to watch large trees > where kqueue will run out of file handles, and because the daemon used > for Spotlight search (mds) triggers many additional events when using > kqueue (I suspect FSEvents will behave better based on file watching > implementation in other languages). > > My hope is that fsnotify will continue to smooth out the differences > between these adapters, as well as File Event Notifications when Solaris > is officially supported. > > For the work I've been doing on fsnotify (on GitHub atm), I've been > coming from a high level, implementing functionality that myself and > others have implemented on top of fsnotify again and again. I don't know > that all that functionality belongs in os/fsnotify vs. an auxiliary > third party library. It's nice to see all the back-and-forth here, and > nice to not be constrained by the existing API (eg. Events channel > rather than Event). > > These are some API ideas I was tossing around 4 months ago, but I'm > certain os/fsnotify can be much better. > https://github.com/howeyc/fsnotify/issues/64 > > https://codereview.appspot.com/48310043/ >
Sign in to reply to this message.
On Mon, Jan 6, 2014 at 11:49 PM, Russ Cox <rsc@golang.org> wrote: > On Mon, Jan 6, 2014 at 11:42 PM, <nj@nathany.com> wrote: > >> Looking at the API so far, it is oriented around files. Inotify is able >> to do a watch on an entire directory rather than just a file. > > > 56 // If file names a directory, operations on files in that directory > 57 // (but not in subdirectories) will also be reported. > > The recursive vs not recursive issue is interesting. What is natively > supported in the various systems? > I think at least Linux, OS X (via FSEvents), and Windows has support for recursive notifications. I don't know about kqueue. I also want recursive watch support, even if it doesn't work on every platform. (however, I'd prefer we return error if recursive watch is asked but not possible on the current platform, instead of trying to emulate it.)
Sign in to reply to this message.
Does Linux have recursive watch support? I haven't been able to find it if so. inotify only tells you about a single file or directory (one level). Does linux provide some other API for it? In the past I've ended up recursively creating inotify watches. On Mon, Jan 6, 2014 at 9:13 PM, minux <minux.ma@gmail.com> wrote: > > On Mon, Jan 6, 2014 at 11:49 PM, Russ Cox <rsc@golang.org> wrote: > >> On Mon, Jan 6, 2014 at 11:42 PM, <nj@nathany.com> wrote: >> >>> Looking at the API so far, it is oriented around files. Inotify is able >>> to do a watch on an entire directory rather than just a file. >> >> >> 56 // If file names a directory, operations on files in that directory >> 57 // (but not in subdirectories) will also be reported. >> >> The recursive vs not recursive issue is interesting. What is natively >> supported in the various systems? >> > I think at least Linux, OS X (via FSEvents), and Windows has support for > recursive notifications. I don't know about kqueue. > > I also want recursive watch support, even if it doesn't work on every > platform. > (however, I'd prefer we return error if recursive watch is asked but not > possible > on the current platform, instead of trying to emulate it.) >
Sign in to reply to this message.
On 2014/01/07 04:49:51, rsc wrote: > On Mon, Jan 6, 2014 at 11:42 PM, <mailto:nj@nathany.com> wrote: > > > Looking at the API so far, it is oriented around files. Inotify is able > > to do a watch on an entire directory rather than just a file. > > > 56 // If file names a directory, operations on files in that directory > 57 // (but not in subdirectories) will also be reported. > > The recursive vs not recursive issue is interesting. What is natively > supported in the various systems? As far as I understand, only Windows and OS X (FSEvents) support recursive directly, whereas fsnotify would need to implement recursive watching internally for inotify and kqueue. I have no idea for Solaris. > Should OS X be using FSEvents instead of kqueue? If so, how? Sam Jacobson had some suggestions here: https://github.com/howeyc/fsnotify/issues/54#issuecomment-24040227 For efficiency, I think it probably should use FSEvents. I hope we can get it in for Go 1.3's code freeze, but at least having done enough research to know we can support it in 1.4 without API changes would be nice. :-) Would it make sense to implement the new API at go.exp/fsnotify? (breaking ~7 users of that import path, though they could switch to the GitHub import). Followed by encouraging some of the ~144 users of the original GitHub import path to switch to the new API prior to Go 1.3?
Sign in to reply to this message.
https://codereview.appspot.com/48310043/diff/40001/src/pkg/os/fsnotify/event.go File src/pkg/os/fsnotify/event.go (right): https://codereview.appspot.com/48310043/diff/40001/src/pkg/os/fsnotify/event.... src/pkg/os/fsnotify/event.go:19: const ( The name I came up with is Triggers (rather than Op). type Triggers uint32 // Trigger types to watch for const ( Create Triggers = 1 << iota Modify Delete Rename ) https://codereview.appspot.com/48310043/diff/40001/src/pkg/os/fsnotify/event.... src/pkg/os/fsnotify/event.go:37: type Event struct { Might I suggest an interface? I started writing unit tests for my new code using a fake event. // Event represents a single file system event type Event interface { IsCreate() bool IsDelete() bool IsModify() bool IsRename() bool Path() string // relative path to the file IsDir() bool }
Sign in to reply to this message.
On Tue, Jan 7, 2014 at 12:17 AM, Caleb Spare <cespare@gmail.com> wrote: > Does Linux have recursive watch support? I haven't been able to find it if > so. inotify only tells you about a single file or directory (one level). > Does linux provide some other API for it? > Oops, my bad. Linux inotify(2) doesn't support it, it was emulated in the user space. OK, if Linux doesn't support it, I guess it makes more sense to do the recursive implementation in an outside package. To implement race-free user-space recursive watches, one need os/fsnotify to implement MkDir Op. > > In the past I've ended up recursively creating inotify watches. >
Sign in to reply to this message.
https://codereview.appspot.com/48310043/diff/40001/src/pkg/os/fsnotify/event.go File src/pkg/os/fsnotify/event.go (right): https://codereview.appspot.com/48310043/diff/40001/src/pkg/os/fsnotify/event.... src/pkg/os/fsnotify/event.go:20: Create Op = 1 << iota how about adding a Mkdir Op to enable implementing a recursive watcher race-free?
Sign in to reply to this message.
On 2014/01/07 05:33:16, minux wrote: > To implement race-free user-space recursive watches, one need os/fsnotify > to implement MkDir Op. I wasn't aware of a MkDir Op, so the user-space recursive watch I just implemented probably has races :-( This is the lengthy pull request I started last September (it would've been better if some intermediary steps got merged): https://github.com/howeyc/fsnotify/pull/65/files Warning: I just got it working on BSD/OSX, I still need to implement IsDir() on Windows and Linux, so it most likely won't compile on those operating systems.
Sign in to reply to this message.
https://codereview.appspot.com/48310043/diff/40001/src/pkg/os/fsnotify/event.go File src/pkg/os/fsnotify/event.go (right): https://codereview.appspot.com/48310043/diff/40001/src/pkg/os/fsnotify/event.... src/pkg/os/fsnotify/event.go:63: // The file must have been previously added with Add. It's not clear to me what "must" means. Does "Remove" panic, return an error or return nil in this case?
Sign in to reply to this message.
Even if all platforms do not natively support recursive watching, I think it would be good to support recursive watching in any case, doing it in user space where necessary. Given the possible performance implications of doing it in user space, it may be useful to allow watching at a single level too (platforms which *only* support recursive watching could filter out other events). https://codereview.appspot.com/48310043/diff/40001/src/pkg/os/fsnotify/event.go File src/pkg/os/fsnotify/event.go (right): https://codereview.appspot.com/48310043/diff/40001/src/pkg/os/fsnotify/event.... src/pkg/os/fsnotify/event.go:37: type Event struct { On 2014/01/07 05:32:31, nathany wrote: > Might I suggest an interface? I started writing unit tests for my new code using > a fake event. > > // Event represents a single file system event > type Event interface { > IsCreate() bool > IsDelete() bool > IsModify() bool > IsRename() bool > Path() string // relative path to the file > IsDir() bool > } I don't see that an interface would add value unless we wanted to be able to hide extra system-specific data behind it. https://codereview.appspot.com/48310043/diff/40001/src/pkg/os/fsnotify/event.... src/pkg/os/fsnotify/event.go:39: File string On 2014/01/07 02:44:04, dfc wrote: > What about adding > > Extra string > > For Op specific data, mainly to deal with the rename case. Not keen on this. If there is to be a rename target (and assuming it's not available on all platforms), I think should be an explicitly named field with well documented semantics.
Sign in to reply to this message.
https://codereview.appspot.com/48310043/diff/40001/src/pkg/os/fsnotify/event.go File src/pkg/os/fsnotify/event.go (right): https://codereview.appspot.com/48310043/diff/40001/src/pkg/os/fsnotify/event.... src/pkg/os/fsnotify/event.go:44: Event <-chan Event What is the plan to handle errors as you read off the OS-specific queue and put them on the Event channel? Perhaps a function instead of a channel? Read() (Event, error) https://codereview.appspot.com/48310043/diff/40001/src/pkg/os/fsnotify/event.... src/pkg/os/fsnotify/event.go:60: func (w *Watcher) Add(file string, op Op) error One thing to keep in mind here is that you will get requests to do OS-specific watches, you'll need to decide how to handle such requests. Requests such as "how come I can't watch for 'closed write' in Linux?" Also, there are some higher-level features that have been requested more than once. Features such as recursive-watches, throttling (some text editors, when editing/saving files generate multiple Write events and users would prefer to receive just one).
Sign in to reply to this message.
https://codereview.appspot.com/48310043/diff/40001/src/pkg/os/fsnotify/event.go File src/pkg/os/fsnotify/event.go (right): https://codereview.appspot.com/48310043/diff/40001/src/pkg/os/fsnotify/event.... src/pkg/os/fsnotify/event.go:48: func NewWatcher() *Watcher On 2014/01/07 02:44:04, dfc wrote: > kqueue and inotify use a file descriptor, so there is a potential that creating > a Watcher may fail. Also, for solaris and plan9 there may be no suitable > facility, so they need to be able to return EPLAN9 or similar. Event ports are a suitable Solaris backend for this.
Sign in to reply to this message.
https://codereview.appspot.com/48310043/diff/40001/src/pkg/os/fsnotify/event.go File src/pkg/os/fsnotify/event.go (right): https://codereview.appspot.com/48310043/diff/40001/src/pkg/os/fsnotify/event.... src/pkg/os/fsnotify/event.go:5: package fsnotify // Package fsnotify does something. https://codereview.appspot.com/48310043/diff/40001/src/pkg/os/fsnotify/event.... src/pkg/os/fsnotify/event.go:18: // These are the file operations that can be watched for and then reported. // These are the file operations that can trigger a notification. https://codereview.appspot.com/48310043/diff/40001/src/pkg/os/fsnotify/event.... src/pkg/os/fsnotify/event.go:45: } type Watcher <-chan Event
Sign in to reply to this message.
https://codereview.appspot.com/48310043/diff/40001/src/pkg/os/fsnotify/event.go File src/pkg/os/fsnotify/event.go (right): https://codereview.appspot.com/48310043/diff/40001/src/pkg/os/fsnotify/event.... src/pkg/os/fsnotify/event.go:20: Create Op = 1 << iota On 2014/01/07 05:34:34, minux wrote: > how about adding a Mkdir Op to enable implementing a recursive watcher > race-free? +1; definitely seems worthwhile, even if it costs us a little bit of performance for Create calls. Should the same be true of Rmdir? Also, it would be really nice if each one of these described the circumstances under which they are intended to fire. https://codereview.appspot.com/48310043/diff/40001/src/pkg/os/fsnotify/event.... src/pkg/os/fsnotify/event.go:37: type Event struct { On 2014/01/07 05:32:31, nathany wrote: > Might I suggest an interface? I started writing unit tests for my new code using > a fake event. > > // Event represents a single file system event > type Event interface { > IsCreate() bool > IsDelete() bool > IsModify() bool > IsRename() bool > Path() string // relative path to the file > IsDir() bool > } Hmm. I'm not sure I like either (a) a static struct or (b) an interface with a ton of methods like reflect.Type. What about an interface: type Event interface { // Is returns true if the event is one of the operations in opMask. Is(opMask Op) bool } and then specific types for each operation? This gives callers the flexibility to simply count/check operations on a file or to type-assert/type-switch it to a specific struct if they need the data about it. https://codereview.appspot.com/48310043/diff/40001/src/pkg/os/fsnotify/event.... src/pkg/os/fsnotify/event.go:60: func (w *Watcher) Add(file string, op Op) error On 2014/01/07 14:24:13, howeyc wrote: > One thing to keep in mind here is that you will get requests to do OS-specific > watches, you'll need to decide how to handle such requests. Requests such as > "how come I can't watch for 'closed write' in Linux?" > > Also, there are some higher-level features that have been requested more than > once. Features such as recursive-watches, throttling (some text editors, when > editing/saving files generate multiple Write events and users would prefer to > receive just one). I second the request for recursive watch. It's tricky enough and common enough that it seems worthwhile to do in the API.
Sign in to reply to this message.
> I second the request for recursive watch. It's tricky enough and common enough > that it seems worthwhile to do in the API. There are a few more requests for recursive watching here: https://github.com/howeyc/fsnotify/issues/56 This evening I began drafting an API doc with the background/proposal/rationale but not much in the way of implementation (as it's being looked at here already). http://goo.gl/MrYxyA I hope it's a useful place to start. Everyone should have comment access, and the Share button should allow you to request edit access.
Sign in to reply to this message.
https://codereview.appspot.com/48310043/diff/40001/src/pkg/os/fsnotify/event.go File src/pkg/os/fsnotify/event.go (right): https://codereview.appspot.com/48310043/diff/40001/src/pkg/os/fsnotify/event.... src/pkg/os/fsnotify/event.go:60: func (w *Watcher) Add(file string, op Op) error On 2014/01/07 22:06:38, kevlar wrote: > On 2014/01/07 14:24:13, howeyc wrote: > > One thing to keep in mind here is that you will get requests to do OS-specific > > watches, you'll need to decide how to handle such requests. Requests such as > > "how come I can't watch for 'closed write' in Linux?" > > > > Also, there are some higher-level features that have been requested more than > > once. Features such as recursive-watches, throttling (some text editors, when > > editing/saving files generate multiple Write events and users would prefer to > > receive just one). Though it wasn't my initial stance, I'm now convinced that the higher-level features belong in a separate higher-level package. Instead of features, os/fsnotify should focus on broad-OS support, uniform behaviour, and stability. Regarding the "Op" option to Add, I don't believe it makes sense to have a filter for file operations while not having several other useful filters. The following thread explains why I think this feature should be removed (or in the least, implemented differently): https://groups.google.com/d/msg/golang-dev/bShm2sqbrTY/PY7rEgljCZwJ > I second the request for recursive watch. It's tricky enough and common enough > that it seems worthwhile to do in the API. A user-space recursive watch can be implemented in a layer above fsnotify, while fsnotify provides uniform access to kqueue, inotify, ReadDirectoryChangesW, etc. The only (potential) disadvantage I see is that the bSubtree option Windows isn't exposed so a user-space recursive watch is required there too. Also, FSEvents is entirely out-of-the-picture (for os/fsnotify), being recursive-only, along with a boatload of other not "generally available" features.
Sign in to reply to this message.
|
