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

Add fsnotify support to gin #15

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

losinggeneration
Copy link

This, at build time, enables fsnotify support for OS's that support it (e.g. Linux.) The this has a slight advantage over the default scanner in that there is almost zero delay between saving and triggering a rebuild.

* The only downside to this is that the fsnotify is in the go.exp
  repository which may break API or move at some point.
@losinggeneration
Copy link
Author

https://github.com/howeyc/fsnotify <- may want to actually use that instead of go.exp (misread that) it also sounds like fsnotify is proposed for Go 1.4 inclusion.

@losinggeneration
Copy link
Author

http://code.google.com/p/go/source/browse/?repo=exp#hg%2Ffsnotify I didn't really look, but it looks like most OS's are supported. Let me know if anything needs cleaned up or changed before you're ready to pull.

@codegangsta
Copy link
Owner

This looks pretty good! Using fsnotify I seem to remember that it does not
recurse into subdirectories. Is this still the case with this code? If so
then this unfortunately does not replicate the filewalker functionality
exactly. We could maintain a list of directories from an initial filewalk,
and also react to CREATE and DELETE events to add/remove the directories
from the list while gin is running.

Again, I don't seem to remember whether or not that is a real concern. I
would love to have fsnotify instead of a filewalker since it is way more
efficient and nearly instant for recompiles.

Sidenote - It would be cool to block serving in the proxy until the app is
done compiling and running again, this way a browser refresh after a save
will block until until we are ready to process the request from the new
server.

On Thu, Mar 13, 2014 at 2:16 PM, Harley Laue [email protected]:

http://code.google.com/p/go/source/browse/?repo=exp#hg%2Ffsnotify I
didn't really look, but it looks like most OS's are supported. Let me know
if anything needs cleaned up or changed before you're ready to pull.

Reply to this email directly or view it on GitHubhttps://github.com//pull/15#issuecomment-37587831
.

@losinggeneration
Copy link
Author

That's a good point. I have a feeling you're right about it not recursing. However, it shouldn't be too hard to make sure all paths are watched (adding/deleting paths as needed.) I'll probably work on that a little later tonight.

Also, like the idea of the sidenote. I guess I would imagine that would be done via a channel. Write down the channel to tell it to block, and then another write tells it to unblock, but I haven't really given it any more thought than that.

@codegangsta
Copy link
Owner

Sounds good. Thanks!

On Thu, Mar 13, 2014 at 2:38 PM, Harley Laue [email protected]:

That's a good point. I have a feeling you're right about it not recursing.
However, it shouldn't be too hard to make sure all paths are watched
(adding/deleting paths as needed.) I'll probably work on that a little
later tonight.

Also, like the idea of the sidenote. I guess I would imagine that would be
done via a channel. Write down the channel to tell it to block, and then
another write tells it to unblock, but I haven't really given it any more
thought than that.

Reply to this email directly or view it on GitHubhttps://github.com//pull/15#issuecomment-37590151
.

* To avoid multiple rebuilds on saving a file, some extra work was
  needed.
@losinggeneration
Copy link
Author

That was a fair bit of work to ensure rebuilds didn't happen multiple times. I think the solution I came up with works well enough. There's basically a flush of the fsnotify events after a build. I noticed the problem when VIM would save a file it would do lots of file movement between the file and temporary files.

@tamasd
Copy link

tamasd commented Apr 26, 2014

It would be really nice to have a config option to ignore certain directories.

My use case is that I have fairly big bower_components directory, with a tons of small files. I think there's a limit on the number of files you can watch through fsnotify.

@losinggeneration
Copy link
Author

@tamasd are you talking about tons of small files that are all Go files? IIRC (and a quick look at the code seems to indicate this as well) is that it's only adding watches to directories and .go files. Would this still be an issue for your browser_components directory? Perhaps I should ask, is this an issue you're already experiencing based on this pull request?

@tamasd
Copy link

tamasd commented Apr 27, 2014

@losinggeneration My bower_components directory (~60 MB, more than 11000 files) contains mostly JavaScript files. When I start gin with your modifications, after a second or two, it exits with the error too many open files. The original gin runs well, but it eats around ~30% CPU.

@codegangsta
Copy link
Owner

I agree that it would be awesome to have a ginignore file to track these kinds of things. Please an issue and I will get around implementing it

Sent from my iPhone

On Apr 27, 2014, at 2:33 PM, Tamás Demeter-Haludka [email protected] wrote:

@losinggeneration My bower_components directory (~60 MB, more than 11000 files) contains mostly JavaScript files. When I start gin with your modifications, after a second or two, it exits with the error too many open files. The original gin runs well, but it eats around ~30% CPU.


Reply to this email directly or view it on GitHub.

@toqueteos
Copy link

Ping ❓

@codegangsta
Copy link
Owner

I'm still not sure about this. It doesn't looks like it works for everybody. I may have to try running it again when I get the chance to

@nalanj
Copy link

nalanj commented Jun 27, 2015

I'm specifically using gin because the other similar tools that use fsnotify don't work over network shares. It'd be nice if both methods were retained if this is ever merged.

@@ -0,0 +1,103 @@
//+build fsnotify
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@commondream The default for this PR is actually to be without fsnotify. It has to be built with go build -tags fsnotify Though, it seems to have a conflict currently.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants