Wrap notify::Watcher to handle vim's write-via-rename #1054
Conversation
| UnwatchInfo::Directory(RecursiveMode::Recursive) => { | ||
| self.recursive_watcher | ||
| .as_mut() | ||
| .expect("FSWatch: inconsistent UnwatchInfo") |
There was a problem hiding this comment.
I honestly don't understand what the possible consequences of not shutting down the server would be in this case but my first thought would be to handle this case and return an error instead of expecting it. The expect calls on the lock calls I think are ok, but this one I'm not so sure.
Other than that this looks good to me, although @autozimu might be better suited to review this one.
There was a problem hiding this comment.
For both of the "inconsistent UnwatchInfo" expect calls, hitting them would mean that one the one hand we think there should be a watcher set up for the unwatched path, but on the other hand we find no such watcher in our data structures. This would mean a bug in the (my) watcher code.
If we don't panic there but just return an error, we are apparently in a situation where we're not sure exactly what watchers are or aren't active on what paths. In the end, I believe this would mean that the language server may get file change events that it didn't ask for, or may not get events for paths that it does have watched.
I'm not sure what your policy is in such situations. It might be possible to merge the two parallel data structures (dirs and unwatch_info), so that this case cannot occur. However, I'm personally not sure exactly how that would play with the Path interface; e.g. do parent and file_name behave predictably and sanely for multiple different ways of writing the same path? That is ultimately why I opted for the unwatch_info strategy.
But by all means, if you disagree this can change :)
There was a problem hiding this comment.
Firstly, thank you for taking time to tackle this.
On the point of expect calls, I would prefer to reduce the number of "hard errors" as much as possible. I do agree that in the unexpected case, we can only issuing an error and nothing more. But halting the whole program is a worse user experience, which also hinder future sophisticated error handling such as restarting the watch thread.
I haven't look through the the whole changes yet and I don't know much further on the parent and file_name topic. Will look further and update here.
There was a problem hiding this comment.
@autozimu Sorry to poll here, but were you intending to do some research and post here again, or are you waiting for me to do anything? I've been happily using my branch for a while now, and it seems to be working (though that doesn't mean the code is perfect, just a confirmation that it's not total crap).
tomsmeding
force-pushed
the
better-file-watch
branch
from
b28d96f
to
ac8717b
Compare
|
Rebased onto latest |
tomsmeding
force-pushed
the
better-file-watch
branch
from
ac8717b
to
d2a8504
Compare
The language server can request a watch on a file that is currently open in vim. If it does so, LanguageClient-neovim adds a file system watcher on that file. Doing this directly via `notify::Watcher` as it did before, however, goes wrong on systems where a file watch is really an inode watch, and vim writes to files by writing to a temporary file and then renaming over the original file. These systems include Linux and macOS.
tomsmeding
force-pushed
the
better-file-watch
branch
from
d2a8504
to
0f78c53
Compare
The language server can request a watch on a file that is currently open in vim. If it does so, LanguageClient-neovim adds a file system watcher on that file. Doing this directly via `notify::Watcher` as it did before, however, goes wrong on systems where a file watch is really an inode watch, and vim writes to files by writing to a temporary file and then renaming over the original file. These systems include Linux and macOS.
|
Merged in via cccb87b Thanks for contributing! |
The language server can request a watch on a file that is currently open in vim. If it does so, LanguageClient-neovim adds a file system watcher on that file. Doing this directly via `notify::Watcher` as it did before, however, goes wrong on systems where a file watch is really an inode watch, and vim writes to files by writing to a temporary file and then renaming over the original file. These systems include Linux and macOS.
The language server can request a watch on a file that is currently open in vim. If it does so, LanguageClient-neovim adds a file system watcher on that file. Doing this directly via
notify::Watcheras it did before, however, goes wrong on systems where a file watch is really an inode watch, and vim writes to files by writing to a temporary file and then renaming over the original file. These systems include Linux and macOS.Related:
notify: unable to watch file edits by vim notify-rs/notify#247