Fix awaitWriteFinish unlink issue #393
Conversation
es128
changed the title
| this._getWatchedDir(sysPath.dirname(path)) | ||
| .remove(sysPath.basename(path)); | ||
| if (this._pendingWrites[path]) { | ||
| this._pendingWrites[path].cancelWait(); |
There was a problem hiding this comment.
cancelWait calls callback(), and the the end of this branch calls callback(err). This will surly cause some inconsistencies.
With this code the
if (err.code == 'ENOENT') return;is not needed.
There was a problem hiding this comment.
Ok, let's try it. 0274e07
There was a problem hiding this comment.
No, the uncaught ENOENT is happening in Travis... going to add that back, but higher up
There was a problem hiding this comment.
Actually, I don't see a reason for cancelWait to call the callback anyway. It doesn't seem like it would make anything happen.
There was a problem hiding this comment.
So we have a race condition between the file polling and the unlink event. one of them should cancel every thing about the file - the (maybe) ongoing timeout, and release the pending async function. In my original code I've relayed on that the unlink event will cancel the wait, and if the polling will come first, it will notice that the file is missing (ENOENT) and do nothing.
Have you seen a case where the unlink event does not arrive at all?
There was a problem hiding this comment.
The issue I observed in the original code that was causing tests to fail regularly was that unlink events were being emitted without any prior add event nor an entry in _pendingWrites. This happened because of the slowness of fs.stat calls, causing misalignment between this._watched tracking and this._pendingWrites (the file shows up in the former, but awf suppressed the corresponding add event from being emitted, then never tracked it because it was gone by the time of the stat).
Hmm, hard to explain.
There was a problem hiding this comment.
Hmm... So maybe the solution is to add the path to _pendingWrites before calling fs.stat? this way, if an unlink event will arrive, it could cancel the wait successfully. I'll try it now
There was a problem hiding this comment.
I did try that. Didn't solve it on its own. The last change in this PR seems to be working
|
For some reason node 0.10 on Windows is very reluctant to cooperate, but tests are now green everywhere else. @alexpusch any concerns with the latest diff? |
| .remove(sysPath.basename(path)); | ||
| if (this._pendingWrites[path]) this._pendingWrites[path].cancelWait(); | ||
| if (err.code !== 'ENOENT') callback(err); | ||
| return; |
There was a problem hiding this comment.
If the file is deleted (err.code === 'ENOENT') callback will never be called.
Here is my try for this fix. I've extracted the creation of _pendingWrites[path] outside of the polling function, and returned the task of deleting the entry to the unlink event (via cancelWait). everything is green
There was a problem hiding this comment.
The callback doesn't necessarily need to be called to do what it currently does. It only emits either an error or the event, and sometimes we want neither. I agree that it's ugly though, and as a next step I wanted to look into refactoring further to deal with cleaning up this control flow.
Please open your version in a PR so we can see how it fares on appveyor, then we can determine which direction seems better.
|
Resolved the issue with e7c3369 |
Looks like I finally squished this one.
// @paulmillr @alexpusch @nono