Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

Fix bug #6609 (Fast file replacement operations can be missed) #6615

Closed
wants to merge 3 commits into from

Conversation

peterflynn
Copy link
Member

Fix bug #6609 --

Dispatch a FileSystem "change" event even when a dir change yields empty added/removed lists - because there's a delay between the change occurring and the time we re-read the dir contents in response, we might see back-to-back changes (e.g. a delete-recreate pair) as a no-op even though something definitely changed. See #6609 for an example bug.

Updated docs to clarify the two different special cases of directory change events. (Also contains an unrelated small docs improvement in FileUtils).

ProjectManager already responds to this special case the way we'd want - it calls FileSyncManager and then does nothing else - so no code changes needed there.

added/removed lists - because there's a delay between the change occurring
and the time we re-read the dir contents in response, we might see
back-to-back changes (e.g. a delete-recreate pair) as a no-op even though
something definitely changed. See #6609 for an example bug.

Updated docs to clarify the two different special cases of directory change
events. (Also contains an unrelated small docs improvement in FileUtils).

ProjectManager already responds to this special case the way we'd want - it
calls FileSyncManager and then does nothing else - so no code changes
needed there.
@peterflynn
Copy link
Member Author

@njx If you want to test this out. I'm still working on a unit test so we may want to hold off on merging though.

Enhance MockFileSystemImpl to allow artificially delaying watcher
notifications in addition to delaying normal callbacks.
@peterflynn
Copy link
Member Author

Unit test added

this._fireChangeEvent(entry, added, removed);
}

// We send a change even if added & removed are both length zero-length. Something may still have changed,
Copy link
Contributor

Choose a reason for hiding this comment

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

typo: "length zero-length"

@njx
Copy link
Contributor

njx commented Jan 23, 2014

Reviewed and tested - just found the typo. Need to retarget this PR at the release branch, though, and then merge it back into master.

@peterflynn
Copy link
Member Author

Closing in order to open a new PR against the release branch

@peterflynn peterflynn closed this Jan 23, 2014
peterflynn added a commit that referenced this pull request Jan 23, 2014
@peterflynn peterflynn deleted the pflynn/dont-ignore-noop-dir-change branch January 23, 2014 22:33
peterflynn added a commit that referenced this pull request Jan 23, 2014
Merge the copy of PR #6615 from release branch back to master
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants