Built-in event filtering of Ops #7

Open
nathany opened this Issue Jun 28, 2014 · 6 comments

Comments

Projects
None yet
3 participants
@nathany
Member

nathany commented Jun 28, 2014

The current mechanism for filtering events is to inspect the Op bitmask:

if event.Op&fsnotify.Create == fsnotify.Create {

In howeyc/fsnotify there is a WatchFlags method to filter operations. WatchFlags was removed in 10e1440 (v0.10.0) because:

  • The implementation required a lot of extra bookkeeping while providing little benefit over filtering events as they were received.
  • As @mstum reported, it wasn't working on Windows, which was due to the bookkeeping not being implemented for files within a directory watch: howeyc/fsnotify#93 (comment).
  • There were no tests for WatchFlags.

@nightlyone pointed out that it still is desirable to specify filters so the kernel doesn't need to wakeup our thread/process for an event we'll just ignore.

Some research (API doc) reveal differences from one OS to the next:

  • kqueue doesn't have a Create Op at all (NOTE_CREATE), much less a filter for it.
  • Windows has a filter for FILE_NOTIFY_CHANGE_FILE_NAME that covers Create, Remove and Rename. An event does indicate which one of these it is, but some user-space filtering is still necessary.

@nathany nathany added the API label Jun 28, 2014

@nathany

This comment has been minimized.

Show comment
Hide comment
@nathany

nathany Jun 28, 2014

Member

My current suggestion is to set the Op filter globally at the Watcher level instead of for each Watch added. Preferably in such a way that it cannot be changed later. Perhaps something like:

watcher, err := fsnotify.NewWatcher(fsnotify.Create|fsnotify.Write)
  • The filter can be passed to each adapter to have the OS filter out events the best it can.
  • A final user-space filter can catch any outliers without requiring the significant amount of bookkeeping of the previous implementation.
  • For the same reasons that directory watches are easier to manage, a user-space recursive watcher will be simpler to implement, whether included in fsnotify or externally.
Member

nathany commented Jun 28, 2014

My current suggestion is to set the Op filter globally at the Watcher level instead of for each Watch added. Preferably in such a way that it cannot be changed later. Perhaps something like:

watcher, err := fsnotify.NewWatcher(fsnotify.Create|fsnotify.Write)
  • The filter can be passed to each adapter to have the OS filter out events the best it can.
  • A final user-space filter can catch any outliers without requiring the significant amount of bookkeeping of the previous implementation.
  • For the same reasons that directory watches are easier to manage, a user-space recursive watcher will be simpler to implement, whether included in fsnotify or externally.
@nightlyone

This comment has been minimized.

Show comment
Hide comment
@nightlyone

nightlyone Jun 30, 2014

I like your proposal of doing the Ops filtering on creation of the watcher.

One usually creates such a watcher with a purpose in mind and the events being delivered are usually suitable for this purpose for the lifetime of the watcher. And after all this is just an optimization. One can always choose a broader scope in order to be sure.

So maybe an additional simple creation function like NewGreedyWatcher might be useful OR an additional complex creation function like NewFilteredWatcher. One of those is meant to be without arguments.

I like your proposal of doing the Ops filtering on creation of the watcher.

One usually creates such a watcher with a purpose in mind and the events being delivered are usually suitable for this purpose for the lifetime of the watcher. And after all this is just an optimization. One can always choose a broader scope in order to be sure.

So maybe an additional simple creation function like NewGreedyWatcher might be useful OR an additional complex creation function like NewFilteredWatcher. One of those is meant to be without arguments.

@nathany nathany referenced this issue in howeyc/fsnotify Jul 8, 2014

Closed

WIP: fixed fsnFlags race condition on linux #103

@nathany nathany added this to the Version 2 API milestone Aug 17, 2014

@nathany nathany modified the milestone: v2 API changes Sep 24, 2014

@nathany nathany added the feature label Sep 24, 2014

@tsuna

This comment has been minimized.

Show comment
Hide comment
@tsuna

tsuna Oct 12, 2016

The current code of Add() contains:

    const agnosticEvents = unix.IN_MOVED_TO | unix.IN_MOVED_FROM |
        unix.IN_CREATE | unix.IN_ATTRIB | unix.IN_MODIFY |
        unix.IN_MOVE_SELF | unix.IN_DELETE | unix.IN_DELETE_SELF

    var flags uint32 = agnosticEvents

So basically the flags are hard-coded to the ones above. Would there be any opposition to adding another API, like:

func (w *Watcher) Add(name string) error {
    return w.AddWithFlags(name, agnosticEvents)
}

func (w *Watcher) AddWithFlags(name string, flags uint32) error {
    // mostly the same code as the current Add() but use flags passed in argument

I need to ignore IN_DELETE_SELF because it's misleading in my case (and causes correctness issues in my code), I only care about IN_DELETE.

tsuna commented Oct 12, 2016

The current code of Add() contains:

    const agnosticEvents = unix.IN_MOVED_TO | unix.IN_MOVED_FROM |
        unix.IN_CREATE | unix.IN_ATTRIB | unix.IN_MODIFY |
        unix.IN_MOVE_SELF | unix.IN_DELETE | unix.IN_DELETE_SELF

    var flags uint32 = agnosticEvents

So basically the flags are hard-coded to the ones above. Would there be any opposition to adding another API, like:

func (w *Watcher) Add(name string) error {
    return w.AddWithFlags(name, agnosticEvents)
}

func (w *Watcher) AddWithFlags(name string, flags uint32) error {
    // mostly the same code as the current Add() but use flags passed in argument

I need to ignore IN_DELETE_SELF because it's misleading in my case (and causes correctness issues in my code), I only care about IN_DELETE.

@nathany

This comment has been minimized.

Show comment
Hide comment
@nathany

nathany Oct 13, 2016

Member

@tsuna The problem with adding a new API like that is that the flags differ from platform to platform. That's partly why I'd like to split out inotify #173 so it can be used independently in special Linux-only cases.

This issue is tracking a more general solution, which may be a ways off yet.

Member

nathany commented Oct 13, 2016

@tsuna The problem with adding a new API like that is that the flags differ from platform to platform. That's partly why I'd like to split out inotify #173 so it can be used independently in special Linux-only cases.

This issue is tracking a more general solution, which may be a ways off yet.

@tsuna

This comment has been minimized.

Show comment
Hide comment
@tsuna

tsuna Oct 13, 2016

I hear you, although I personally don't think it's a big problem that the flags differ across the platforms, since this API would just take a uint32 and what values make sense for this API could be platform specific. So this would be a more advanced API for those needing the extra control.

Now when I reviewed the open issues I missed #173, that's obviously a more elegant solution, and at this rate between this bug and #97 and #123, it's very hard for me to get correctness out of fsnotify, so I was considering writing my own little inotify wrapper anyway, so I'm glad to hear that this is being considered.

For now I think I'll just fork the project to quickly fix/hack the bugs that are impacting me.

tsuna commented Oct 13, 2016

I hear you, although I personally don't think it's a big problem that the flags differ across the platforms, since this API would just take a uint32 and what values make sense for this API could be platform specific. So this would be a more advanced API for those needing the extra control.

Now when I reviewed the open issues I missed #173, that's obviously a more elegant solution, and at this rate between this bug and #97 and #123, it's very hard for me to get correctness out of fsnotify, so I was considering writing my own little inotify wrapper anyway, so I'm glad to hear that this is being considered.

For now I think I'll just fork the project to quickly fix/hack the bugs that are impacting me.

@nathany

This comment has been minimized.

Show comment
Hide comment
@nathany

nathany Oct 13, 2016

Member

Pull requests are welcome, and once we have more people reviewing and merging pull requests, things should start to move faster.

I think breaking out the low-level platform specific libraries (such as #173) will also help there. It should allow experts in inotify to work on that without worrying about the cross-platform API, and others to tackle cross-platform consistency without dealing with all the low-level details.

Member

nathany commented Oct 13, 2016

Pull requests are welcome, and once we have more people reviewing and merging pull requests, things should start to move faster.

I think breaking out the low-level platform specific libraries (such as #173) will also help there. It should allow experts in inotify to work on that without worrying about the cross-platform API, and others to tackle cross-platform consistency without dealing with all the low-level details.

tsuna added a commit to aristanetworks/fsnotify that referenced this issue Oct 16, 2016

Allow specifying flags when adding a watch.
On Linux, implementing a recursive watch with IN_DELETE_SELF is
virtually impossible.  Since the API doesn't offer any way to pick
which flags to use (see issue #7), such an API is needed, and this
is a crude but effective attempt at adding one.

0x1997 added a commit to 0x1997/fsnotify that referenced this issue Dec 6, 2016

Allow specifying flags when adding a watch.
On Linux, implementing a recursive watch with IN_DELETE_SELF is
virtually impossible.  Since the API doesn't offer any way to pick
which flags to use (see issue #7), such an API is needed, and this
is a crude but effective attempt at adding one.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment