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

Event Processing Pipeline (WIP) #65

Closed
wants to merge 30 commits into from
Closed

Conversation

@nathany
Copy link
Contributor

nathany commented Sep 11, 2013

This introduces a pipeline for processing events before forwarding them to the Event channel.

  • The trigger pipeline step is used to filter create, modify, delete, rename events. It's logic is an extraction of the code that was in purgeEvents().
  • The hidden pipeline step filters hidden files and directories
  • The pattern pipeline step filters based on shell file pattern, eg. ".go,.c"
  • The throttle pipeline step filters trailing events on the same file
  • The logging step logs all new events (and the verbose flag is used elsewhere)

Initially I avoided changing the exported API, but since I began adding additional pipeline steps, I've started to implement changes as originally proposed in #64.

The tests pass on OS X, Linux, BSD and Windows and I have been testing it from Looper as well. I have seen some intermittent failures in TestFsnotifySubDir on OS X (#67).

  • Update usage information in README and example_test (GoDoc).
  • how to handle fallbacks when pipeline (was fsnFlags) was not found in the Watcher structure on Linux/BSD

Event

  • add IsDir() to Event interface/FileEvent implementation (do a separate PR) NEXT

Recursive

  • The auto watch pipeline step needs to watch a folder with the same pipeline/options
  • Need to handle errors from the auto watch (put on the Error channel?)
  • Remove watch recursively (the externalWatches addition to kqueue may help here?)
  • Tests

Hidden

  • does Windows need special handling for filtering hidden files/folders
  • handle hidden edge case, request a watch on a hidden folder with Hidden:false
  • make hidden option more clear (Keep/IgnoreHidden?)

Throttle

  • note behaviour and configuration may change to match FSEvents adapter in the future #54

Verbose

  • determine more specifically what it should/shouldn't log
  • should it work with other logging packages? can we add that later easily enough? (just more Options)

API

  • should the notifier interface be public and what should it be called? Either way the Event channel must remain a type of FileEvent to avoid breaking changes.
  • continue discussion of public API #64 before merging to master? come back to channels vs. iterators and other API changes as part of the move to os/fsnotify.

Tests

  • Keep current integration tests for previous API and write up new ones to test against new APIs.
wmut sync.Mutex // Protects access to watches.
fsnFlags map[string]uint32 // Map of watched files to flags used for filter
fsnmut sync.Mutex // Protects access to fsnFlags.
pipelines map[string]pipeline // Map of watched files to pipelines used for filtering

This comment has been minimized.

Copy link
@nathany

nathany Sep 11, 2013

Author Contributor

I was learning towards map[string]*pipeline here to save memory when inheriting from the parent directory, but I never figured out the locking/copying that would be necessary for processEvent(). Any suggestions or is this good enough for now?

This comment has been minimized.

Copy link
@nathany

nathany Sep 22, 2013

Author Contributor

I'm really not that well versed in Mutexes and pointers.

} else {
w.fsnFlags[filePath] = FSN_ALL
w.pipelines[filePath] = pipeline{fsnFlags: FSN_ALL}

This comment has been minimized.

Copy link
@nathany

nathany Sep 11, 2013

Author Contributor

This fallback probably isn't going to work so well once the pipeline carries more state. (in 2 other places) What should be done?

pipeline.go Outdated
A pipeline to process events
*/
type pipeline struct {
fsnFlags uint32 // flags used for triggers() filter

This comment has been minimized.

Copy link
@nathany

nathany Sep 11, 2013

Author Contributor

additional configuration and intermediary state would go here, such as:

patterns []string // for matching on shell patterns
latency time.Duration // for throttling
events map[string]*eventOnFile // time.Time for throttling events
steps []step // which filters are enabled
nathany added 2 commits Sep 21, 2013
keep integration tests as-is to prove the deprecated APIs still function
} else {
w.fsnFlags[filePath] = FSN_ALL
w.pipelines[filePath] = pipeline{}

This comment has been minimized.

Copy link
@nathany

nathany Sep 21, 2013

Author Contributor

not entirely sure what to do with these fallbacks in BSD and Linux, especially as more steps are added to the pipeline

nathany added 9 commits Sep 21, 2013
might want some Windows-specific tests
use notes for deprecations
http://godoc.org/go/doc#Note
finding i need to do "vagrant up --provision" instead of just vagrant up
would like tags to match. semantic versions might be nice :-)
should not have been exported

#64 (comment)
@nathany
Copy link
Contributor Author

nathany commented Oct 24, 2013

@howeyc Have you had a chance to review this?

I'll plan on implementing a recursive watcher and autowatch pipeline step this weekend.

@howeyc
Copy link
Owner

howeyc commented Oct 25, 2013

Yes I have and I like it so far. What do you think about making your "notifier" interface public adding some kind of NextEvent() function?

Something like this: https://gist.github.com/howeyc/7156865

@nathany
Copy link
Contributor Author

nathany commented Oct 25, 2013

Looks good to me.

I think my big challenge for the recursive watcher will be ensuring all the options for the pipeline steps are available to all the folders.

I'd really like to tease apart the code for fsnotify and the inotify/kqueue/etc. adapters, but I'd rather do a big internal refactoring as a separate pull request.

nathany added 3 commits Oct 27, 2013
Tracks external watches

Conflicts:
	fsnotify_bsd.go
tried Notifier/notification, but it's just so long... and mostly inconsistent.

surprisingly this doesn't conflict with the Event channel in a Watcher
nathany added 3 commits Oct 27, 2013
* autowatch step needs to add watches
* remove watches recursively
passing the pipeline instead of options
Conflicts:
	CHANGELOG.md
@nathany nathany mentioned this pull request Nov 30, 2013
Conflicts:
	AUTHORS
	CHANGELOG.md
@nathany
Copy link
Contributor Author

nathany commented Nov 30, 2013

Sorry this has been taking me so long. I'm going to try to get recursive watching wrapped up this weekend and replace out the custom one I built for Looper to get some "real world" use.

If anyone else wants to try out this code from a branch, just add a remote for git@github.com:gophertown/fsnotify.git to your local copy of howeyc/fsnotify and fetch the pipeline branch. See "for other team members" in contributing for details.

Obviously it's going to take a bit longer to complete... with the number of unchecked boxes and my current pace.

It would be great to have some others try out the code and API changes on a branch before this gets merged to master. /cc @robfig

@nathany nathany mentioned this pull request Nov 30, 2013
6 of 9 tasks complete
t.Fatalf("failed to create subdir in test directory: %s", err)
}

time.Sleep(50 * time.Millisecond) // give system time to detect folder before creating file

This comment has been minimized.

Copy link
@nathany

nathany Jan 7, 2014

Author Contributor

Really this delay shouldn't be necessary. I wonder if this is an example of a race condition that minux mentioned (events out of order or the file event goes missing?).

https://codereview.appspot.com/48310043/ # 16

@nathany
Copy link
Contributor Author

nathany commented Jan 11, 2014

minux suggested a user-defined function to replace Pattern and Hidden. It's essentially a func filter(path string) bool. It's more flexible. Want to exclude files based on a pattern? Sure thing. Obviously it moves a fair bit of code (with lovely tests) into the user code, but it should be easy enough to write a small wrapper with the same API.

What about adding multiple filters? These filters are slightly different than the Event pipeline:

  • Run when adding a watch to skip folders/files that don't need to be watched (subdirectories, sendDirectoryChangeEvents)
  • Run as part of the pipeline on the Event.Path().

Also, heard the suggestion of making the Event interface smaller.

@nathany
Copy link
Contributor Author

nathany commented Jan 16, 2014

It looks like these features won't be in os/fsnotify, but development could be moved to a wrapper library.

https://github.com/libgo/fsnotify_ext (not up-to-date with the changes here)

@nathany nathany closed this Jan 16, 2014
@nathany
Copy link
Contributor Author

nathany commented May 17, 2014

I'm not sure if these ideas will become part of os/fsnotify, but perhaps I'll build a library that does encompass these ideas/code.

@nathany nathany reopened this May 17, 2014
@nathany nathany closed this Jun 29, 2014
actgardner pushed a commit to scalingdata/fsnotify that referenced this pull request Apr 2, 2015
sending done would close w.kq before Remove had a chance to remove the watches with EV_DELETE, resulting in a file handle leak.

ref howeyc#59

also make Close() report the first error returned by Remove and continue.

closes howeyc#65
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.