Skip to content

coffee --watch stops detecting changes to files #1853

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

Closed
kgn opened this issue Nov 11, 2011 · 40 comments
Closed

coffee --watch stops detecting changes to files #1853

kgn opened this issue Nov 11, 2011 · 40 comments
Assignees

Comments

@kgn
Copy link

kgn commented Nov 11, 2011

While using the head revision of CoffeeScript and a build of NodeJS from yesterday I encountered a failure while running compile --watch.

Assertion failed: (0), function uv_close, file src/unix/core.c, line 149.

This happened after I discarded changes on a CS file in a git repo. I tested that this was reproducible.

This appeared to be bubbling up from NodeJS so I rebuilt from the head revision. There were some commits about uv and fs.watch so I hoped this would fix the problem. Unfortunately it only hid the issue, I re-ran my test and didn't see the assertion but watch stopped working.

@TrevorBurnham
Copy link
Collaborator

Sigh. Sorry about this. Here's the story so far:

  • In Node 0.6.0, fs.watchFile does not run on Windows (see Node: use fs.watch api instead #1803) while fs.watch runs everywhere; so for 1.1.3, we switched to the new fs.watch.
  • But even though 0.6.0 is a "stable" release, fs.watch is riddled with bugs, which we've been trying to work around (Re-fs.watching files on rename event #1847). The nastiest is the one you discovered: That it crashes the entire process if you point fs.watch at a non-existent file! That's now fixed on Node master after I reported it here.

Could you show me your test code? The current master should only stop watching a file if a rename event is triggered on it and the file no longer exists at the same location as before. So if you delete a file and then replace it, then yeah, it'll no longer be watched, which is different from the old behavior because fs.watchFile simply said "I'm going to check the mtime on this file every 500 milliseconds." In principle, fs.watch allows us to detect new files by pointing it at a directory but, well, see #1847.

@kgn
Copy link
Author

kgn commented Nov 11, 2011

There isn't any specific code that causes this issue, it is simply caused by discarding uncommitted changes in a git repo that CS is watching. I put together this video to show what I'm seeing: http://vimeo.com/31979347

In this video I'm using the latest revisions of CoffeeScript and NodeJS as of this morning:
CoffeeScript5bf8b422f80b6a2acceaa268719c7f29a724e1a7
NodeJS e0f10ecfd9a3d957398382a51f664b1562501a7e

I'm not seeing the assertion anymore, but watch still dies when the changes are discarded.

@TrevorBurnham
Copy link
Collaborator

Thanks for the detailed report. I'll try to replicate and figure this out ASAP. In the meantime, you might want to work off a fork that uses fs.watchFile (see my reopened pull request).

On Nov 11, 2011, at 11:00 PM, David [email protected] wrote:

There isn't any specific code that causes this issue, it is simply caused by discarding uncommitted changes in a git repo that CS is watching. I put together this video to show what I'm seeing: http://vimeo.com/31979347

In this video I'm using the latest revisions of CoffeeScript and NodeJS as of this morning:
CoffeeScript5bf8b422f80b6a2acceaa268719c7f29a724e1a7
NodeJS e0f10ecfd9a3d957398382a51f664b1562501a7e

I'm not seeing the assertion anymore, but watch still dies when the changes are discarded.


Reply to this email directly or view it on GitHub:
#1853 (comment)

@kgn
Copy link
Author

kgn commented Nov 12, 2011

Cool, let me know if I can provide any more assistance. This isn't a show stopping issue for me, watch works way better in the latest revision then in the 1.1.3 release. This is the only issue I've run into in 1.1.4-pre and it only happens in this rare case of uncommitting changes.

@TrevorBurnham
Copy link
Collaborator

This is going to be hard to fix... the problem is that git is deleting, then creating the files, as two distinct, atomic events. When our watch function detects the deletion, it asks, "OK, is there a replacement file there? No? Then I'm unbinding myself." You can't fs.watch a filepath and have it keep watching that path no matter what, the way you could with fs.watchFile (which simply checked "Does a file exist at this path?" every n milliseconds—something we want to avoid if possible).

The only way to solve this without polling is to watch the parent directory of each watched file as well.

@ghost ghost assigned TrevorBurnham Nov 12, 2011
@kgn
Copy link
Author

kgn commented Nov 12, 2011

If you watch the parent directory would it be possible to detect new CS files to compile? This is something I always wished watch did.

@michaelficarra
Copy link
Collaborator

@InScopeApps: See @TrevorBurnham's comment over at #1847 saying we could do exactly that. Whether we want to do that is the question.

@TrevorBurnham
Copy link
Collaborator

Whether we want to do that is the question.

I think it's fairly obvious that we do. The feature has been requested multiple times, and Jeremy's objection has always been "Your laptop battery life will be the better off for it." That objection no longer applies if we're scanning the directory anyway (and only when its contents change).

In fact, the implementation that recognizes all new files is more elegant and efficient than the one that just fixes this issue. Not recognizing new files would require us to ask on each directory event, "Did this file exist during our first sweep?"

@michaelficarra
Copy link
Collaborator

I was making a reference to #1846, putting off the adoption of this new API until it's actually stable. But yeah, if we want to write all that code for our "simple" compilation tool, I wouldn't mind reviewing the pull request.

@kgn
Copy link
Author

kgn commented Nov 14, 2011

@TrevorBurnham I just tried your branch and it looks like it does fix this issue!

It doesn't seem to compile new CS files though, is this behavior not being added?

@TrevorBurnham
Copy link
Collaborator

@InScopeApps Detecting new files in directories with fs.watch is something I've been working on, but it's not wise to do so right now (at least under OS X) due to several show-stopping fs.watch bugs I've reported on the Node issue tracker and noted in #1846.

@kgn
Copy link
Author

kgn commented Nov 15, 2011

Makes sense, too bad about fs.watch :(

@timmolendijk
Copy link

--watch doesn't work for me at all. I've tried many different syntaxes; most of the examples under http://jashkenas.github.com/coffee-script/#installation.

ubuntu-10.10$ node -v; coffee -v
v0.6.3
CoffeeScript version 1.1.3

It used to work fine pre CS 1.1.3

@TrevorBurnham
Copy link
Collaborator

Try installing the latest master. There's an issue where some programs "save" files by moving a temporarily file on top of the existing one, rather than writing to the existing file directly, so that may be what you're seeing. On the current master, coffee tries to work around this.

On Nov 27, 2011, at 9:06 PM, Tim [email protected] wrote:

--watch doesn't work for me at all. I've tried many different syntaxes; most of the examples under http://jashkenas.github.com/coffee-script/#installation.

$ node -v; coffee -v
v0.6.3
CoffeeScript version 1.1.3


Reply to this email directly or view it on GitHub:
#1853 (comment)

@timmolendijk
Copy link

@TrevorBurnham: Thanks for the tip. I'm running 1.1.4-pre (current master) now. Unfortunately this doesn't change anything. Compiling works fine, compiling with --watch seems to work fine (process compiles at start-up and doesn't end), but changes to the source are simply not detected.

@timmolendijk
Copy link

Ahh found something interesting!

The problem I described only occurs in Dropbox folders. So I guess this is basically an issue with node.js fs.watch?

Funny that this behavior didn't exist back when coffee didn't depend on fs.watch for this…

@timmolendijk
Copy link

Hmm, it turns out to be even more complicated. But I think this is an issue with node.js so will report at nodejs/node-v0.x-archive#1986.

@timmolendijk
Copy link

Looks like watching for file changes is rather tricky business. Wouldn't it be an idea to go for a more dumbed down and therefore simpler approach instead? Compass's command line utility offers a --poll parameter in addition to its watch parameter, which is (as far as I know) a flag to enable brute-force change monitoring by simple polling. It's a nice way of not having to deal with all the nasty edge cases of detecting file changes on different platforms by providing a fallback solution.

@TrevorBurnham
Copy link
Collaborator

Yes, just doing fs.stat every 500ms might well be the simplest solution. In principle, though, fs.watch is more efficient (polling is impractical for large numbers of files) and will allow us to detect the creation of new files in watched directories.

On Nov 27, 2011, at 10:35 PM, Tim [email protected] wrote:

Looks like watching for file changes is rather tricky business. Wouldn't it be an idea to go for a more dumbed down and therefore simpler approach instead? Compass's command line utility offers a --poll parameter in addition to its watch parameter, which is (as far as I know) a flag to enable brute-force change monitoring by simple polling. It's a nice way of not having to deal with all the nasty edge cases of detecting file changes on different platforms by providing a fallback solution.


Reply to this email directly or view it on GitHub:
#1853 (comment)

@timmolendijk
Copy link

@TrevorBurnham: Note that the way coffee solved this before 1.1.3 worked for me, in all cases as described in the node issue. Yes, it was fairly limited, but at least it worked. I get that fs.watch is the more elegant approach, but the current reality is that I can't watch for changes at all.

(Thanks for the suggestions for Jitter and WatchIt. Those look like potential solutions. But clearly it's far from an ideal situation having to resort to such far-fetched measures just to get a tiny part of a tool chain to work properly.)

@TrevorBurnham
Copy link
Collaborator

@timmolendijk To be clear, I agree that we should be using fs.watchFile on OS X until fs.watch is more stable. Jeremy, however, does not. See the discussion at #1846.

@michaelficarra
Copy link
Collaborator

As do I. fs.watch is nowhere near ready for use.

@TrevorBurnham
Copy link
Collaborator

Looks like Jeremy is reconsidering fs.watch. See #1941.

@jashkenas
Copy link
Owner

Trevor and Michael, please take a look at the above patch...

I've (hopefully) fixed the issue by waiting to re-watch the file after a rename for a quarter second. This behaves fairly well on MacVim, on my machine -- although I am able to get it to crash with an ENOENT by leaning on the save key for a couple seconds -- in regular use, this seems to be reliable.

@zonak
Copy link

zonak commented Dec 21, 2011

Still experiencing the same behavior:

  • OS X Lion 10.7.2
  • Node v0.6.6
  • CoffeeScript 1.2.0
  • Using TextMate 2:
    When doing changes, only the first time the changes are detected and acted upon. Subsequent changes to the same file are not detected.
  • Using TextEdit
    Changes are detected properly every time the file is updated.

When the process gets stuck after using TextMate 2 using TextEdit after that won't help - probably again caused by whatever TM2 does to the file after the initial change is done.

@jashkenas
Copy link
Owner

How about TextMate 1?

That's the same version of OSX / Node / Coffee that I'm using...

@zonak
Copy link

zonak commented Dec 21, 2011

Just tried installing TM1, node, CS on a machine I don't use for dev work - wanted to be sure that it isn't something local.

Tried it with TM1 and it works fine - changes are detected every time the file is updated.

The latest revision of TM2 allows the two versions to co-exist so tried it with TM2 and same results as on my dev machine - only the first time the change is detected and any subsequent change is not.

Whatever TM2 is doing is messing things up.

Let me know if you need me to try something else and thank you for checking this out.

@zonak
Copy link

zonak commented Dec 21, 2011

BTW

Using Trevor's jitter which I think uses fs.watchFile() works fine - does what I need until this is resolved.

@TrevorBurnham
Copy link
Collaborator

So, what's going on is pretty alarming: Somehow, the way TM2 saves files causes a change event to be emitted... and then the file emits no further events, as if it had been deleted. See #1963 for a pull request with a workaround.

@TrevorBurnham
Copy link
Collaborator

@zonak Try the current master.

@zonak
Copy link

zonak commented Dec 22, 2011

Yep ... works great. Thank you for your attention on this.

@infininight
Copy link

Is this issue showing up on HFS+ drives? Textmate 2 uses exchangedata() for atomic saving — this should preserve the inode, but if on a third party network w/o atomic swap then it creates a new file. You can check if you inode is the same by doing ls -i «file» before and after a save.

– Michael, Macromates

@zonak
Copy link

zonak commented Dec 23, 2011

Yep, the file system is Journaled HFS+. Trevor's latest changes do resolve the issue and thank you for looking into this.

@TrevorBurnham
Copy link
Collaborator

Thanks for the info, Michael. I'll pass it on to the Node team, and hopefully they'll be able to fix the fs.watch implementation.

On Dec 23, 2011, at 4:07 AM, Michael [email protected] wrote:

Is this issue showing up on HFS+ drives? Textmate 2 uses exchangedata() for atomic saving — this should preserve the inode, but if on a third party network w/o atomic swap then it creates a new file. You can check if you inode is the same by doing ls -i «file» before and after a save.

  • Michael, Macromates

Reply to this email directly or view it on GitHub:
#1853 (comment)

@raganwald
Copy link
Contributor

Don’t know if this is related, but some “watch’ algorithms look for increases in the file’s last modified property. When you do a git revert, the last modified property is reverted as well, and the watch algorithm won’t catch the revert because the last modified has decreased instead of increased. The correct implementation seems to be to watch for a changing fingerprint or for any change to the last modified property.

Apologies if this is wrong and/or irrelevant to this specific issue...

@jashkenas
Copy link
Owner

Nice catch. We should definitely fix it to be is not instead of >. Re-opening.

@jashkenas jashkenas reopened this Dec 30, 2011
@TrevorBurnham
Copy link
Collaborator

Aren't we doing that already?

    return rewatch() if prevStats and stats.size is prevStats.size and
      stats.mtime.getTime() is prevStats.mtime.getTime()

@jashkenas
Copy link
Owner

Oops -- nevermind then.

@raganwald
Copy link
Contributor

@TrevorBurnham Thanks, my bad for not reviewing the source before commenting!

@TrevorBurnham
Copy link
Collaborator

@raganwald No prob; it's good to know for the future.

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

No branches or pull requests

8 participants