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

Fix ENOENT errors in _addToWatched #60

Closed
wants to merge 3 commits into from
Closed

Fix ENOENT errors in _addToWatched #60

wants to merge 3 commits into from

Conversation

zeripath
Copy link
Contributor

@zeripath zeripath commented Dec 7, 2013

This fixes #45.

The additional test simply tries to run gaze on a filesystem where there is a broken symlink.

A more complete test-set would try to cause the race condition whereby a file that is due to be stat'd is deleted after it is queued but before fs.statSync is run however I am unsure how to go about creating that test.

@zeripath
Copy link
Contributor Author

zeripath commented Dec 7, 2013

Should also fix #58

@zeripath
Copy link
Contributor Author

zeripath commented Dec 7, 2013

@zeripath
Copy link
Contributor Author

Hi @shama, is there anything you would like me to do to improve this pull-request?

@shama
Copy link
Owner

shama commented Dec 20, 2013

Thanks for the pull request. I'm not sure catching and ignoring the error within gaze is the best solution at this point. I'm in the process of writing a layer for gaze to sit upon to take care of some other open issues. I'll take a look at this and the ENOENT errors a bit more once that platform is done as it will likely fix the race conditions that can occur at present.

@zeripath
Copy link
Contributor Author

So thinking about it, the error that is being caught tells us that the file
that we are trying to stat is non-existent. At that point gaze isn't doing
anything with the file and is just trying to see if it is a directory. Only
if the file is a directory would gaze do anything with the file. Having the
file be non-stat'able doesn't change anything else, and can't cause any
further problems, it's simply that the file is either a broken link or was
deleted between being the directory being read and gaze getting round to
trying to stat it. The only sensible thing to do is log it or ignore it.
There wasn't really a way to log this without gaze throwing an exception,
so I just ignored it.

I can take a look at finding a way to log this if you'd like, but this
really is a killer bug for me and other users of emacs. I've also been
bitten by the race condition previously, (back when I was using vim more),
as if you have gaze watching whilst you refactor, you or a grunt task will
occasionally delete a file just before the stat thus throwing an ENOENT.
This fix stops that from happening.

Many thanks,

Andy

On 20 December 2013 17:20, Kyle Robinson Young notifications@github.comwrote:

Thanks for the pull request. I'm not sure catching and ignoring the error
within gaze is the best solution at this point. I'm in the process of
writing a layer for gaze to sit upon to take care of some other open
issues. I'll take a look at this and the ENOENT errors a bit more once that
platform is done as it will likely fix the race conditions that can occur
at present.


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

Andrew Thornton
art27@cantab.net

@zeripath
Copy link
Contributor Author

Hi,

I've updated the fix to emit a warning. In reality this is rather the same as the previous solution as emit('warning') appears to be being squashed somewhere.

I note that further up the library in helper.forEachSeries there is a empty catch section that doesn't check the type of error. It appears that this is a race-condition fix for a non-existent file. In this case, a more appropriate fix might be to check the type of error thrown, and emit a warning as per the latest if there is ENOENT (which is what the race-condition should throw.) Otherwise, it should likely just emit error.

I will happily rewrite _handleWarning to use util.log() if you would like rather than emit error.

How's the work going on the sublayer?

@shama
Copy link
Owner

shama commented Jan 13, 2014

Cool thanks, I'll take another look at this once v0.5 is ready. It's getting close. Check out the v0.5 branch. Currently there is 1 failing test on linux, platform needs more tests and the pathwatcher dep needs to get packaged up into the library.

@glasserc
Copy link

Another option might be to use fs.lstatSync to determine whether the file in question is a symlink, and if so, use fs.readlinkSync to determine if the file being pointed to exists.

@carljm
Copy link

carljm commented Oct 5, 2015

I know there are big issues in gaze master that are holding up 0.6, but it would sure make a lot of people's lives a lot easier if this patch could be applied to the 0.5.1 tag and a 0.5.2 released that is identical to 0.5.1 but includes this fix. I'd be happy to do it myself, given appropriate npm collaborator status.

The workarounds that we are reduced to in the meantime are pretty horrific: https://github.com/jacksonrayhamilton/fix-gaze

shama added a commit that referenced this pull request Oct 5, 2015
@shama
Copy link
Owner

shama commented Oct 5, 2015

@carljm The fix has been backported and published as 0.5.2. Please let me know if that issue still occurs. Thanks!

@carljm
Copy link

carljm commented Oct 5, 2015

0.5.2 does fix the problem -- thank you!!

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

Successfully merging this pull request may close these issues.

Automatically adding directories causing issues with hidden files
4 participants