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

Drive GetModificationTime using watched file events #1487

Merged
merged 7 commits into from Mar 6, 2021

Conversation

pepeiborra
Copy link
Collaborator

This removes the need to poll the file system for source files (we still do it for interface files).

The implementation is remarkably simple, as it uses the watch notifications only to dirty the Shake node, but we still reuse the existing code to compute the result of the rule.

Closes #803

@pepeiborra
Copy link
Collaborator Author

We need to change lsp-test to send textDocument/didChange notifications in changeDoc. @bubba @wz1000 any chances you could make that change and upload a point release to Hackage?

@wz1000
Copy link
Collaborator

wz1000 commented Mar 3, 2021

Doesn't it already send those notifications?

@pepeiborra
Copy link
Collaborator Author

Doesn't it already send those notifications?

Ok, it does actually. The Haddocks are a lie...

@pepeiborra
Copy link
Collaborator Author

Ok, so the notification I had in mind is ‘workspace/didChangeWatchedFiles’ and it's only needed when changing non FOIs, which some tests do, in which case it's probably best to just sent it manually from the test

@wz1000
Copy link
Collaborator

wz1000 commented Mar 3, 2021

BTW, I'm pretty sure vscode doesn't send file-changed notifications if the file is changed outside the editor. To support that case (other tools, git etc. can change files), I think it would be good to directly listen for file system events.

This would also let us handle iface files and similar.

fsnotifys interface seems easy enough to hook up into this: https://hackage.haskell.org/package/fsnotify-0.3.0.1/docs/System-FSNotify.html#v:watchTree

(There is also a slightly selfish reason for this, in that my editor doesn't send these events at all)

@pepeiborra
Copy link
Collaborator Author

pepeiborra commented Mar 3, 2021

It seems to be working for me. This is the message sent by VSCode on a git branch checkout:

2021-03-03 22:34:43.674318 [ThreadId 5] DEBUG haskell-lsp.parseOne:	---> {"jsonrpc":"2.0","method":"workspace/didChangeWatchedFiles","params":{"changes":[{"uri":"file:///Users/pepeiborra/scratch/ide/ghcide/test/data/plugin/KnownNat.hs","type":1},{"uri":"file:///Users/pepeiborra/scratch/ide/ghcide/test/data/plugin/RecordDot.hs","type":1},{"uri":"file:///Users/pepeiborra/scratch/ide/ghcide/src/Development/IDE/Core/FileExists.hs","type":2},{"uri":"file:///Users/pepeiborra/scratch/ide/ghcide/src/Development/IDE/Core/FileStore.hs","type":2},{"uri":"file:///Users/pepeiborra/scratch/ide/ghcide/src/Development/IDE/Core/Rules.hs","type":2},{"uri":"file:///Users/pepeiborra/scratch/ide/ghcide/src/Development/IDE/Core/Shake.hs","type":2},{"uri":"file:///Users/pepeiborra/scratch/ide/ghcide/src/Development/IDE/LSP/Notifications.hs","type":2},{"uri":"file:///Users/pepeiborra/scratch/ide/ghcide/test/exe/Main.hs","type":2}]}}
2021-03-03 22:34:43.674467 [ThreadId 27] DEBUG hls:	Watched file events: [FileEvent {_uri = Uri {getUri = "file:///Users/pepeiborra/scratch/ide/ghcide/test/data/plugin/KnownNat.hs"}, _xtype = FcCreated},FileEvent {_uri = Uri {getUri = "file:///Users/pepeiborra/scratch/ide/ghcide/test/data/plugin/RecordDot.hs"}, _xtype = FcCreated},FileEvent {_uri = Uri {getUri = "file:///Users/pepeiborra/scratch/ide/ghcide/src/Development/IDE/Core/FileExists.hs"}, _xtype = FcChanged},FileEvent {_uri = Uri {getUri = "file:///Users/pepeiborra/scratch/ide/ghcide/src/Development/IDE/Core/FileStore.hs"}, _xtype = FcChanged},FileEvent {_uri = Uri {getUri = "file:///Users/pepeiborra/scratch/ide/ghcide/src/Development/IDE/Core/Rules.hs"}, _xtype = FcChanged},FileEvent {_uri = Uri {getUri = "file:///Users/pepeiborra/scratch/ide/ghcide/src/Development/IDE/Core/Shake.hs"}, _xtype = FcChanged},FileEvent {_uri = Uri {getUri = "file:///Users/pepeiborra/scratch/ide/ghcide/src/Development/IDE/LSP/Notifications.hs"}, _xtype = FcChanged},FileEvent {_uri = Uri {getUri = "file:///Users/pepeiborra/scratch/ide/ghcide/test/exe/Main.hs"}, _xtype = FcChanged}]

@pepeiborra
Copy link
Collaborator Author

BTW your client will continue to work with HLS, as long as it advertises its capabilities correctly. If it doesn't, we can add a flag

@pepeiborra
Copy link
Collaborator Author

pepeiborra commented Mar 3, 2021

Finally, I completely agree with the LSP spec on the topic of setting up our own file watches:

Servers are allowed to run their own file system watching mechanism and not rely on clients to provide file system events. However this is not recommended due to the following reasons:

  • to our experience getting file system watching on disk right is challenging, especially if it needs to be supported across multiple OSes.
  • file system watching is not for free especially if the implementation uses some sort of polling and keeps a file system tree in memory to compare time stamps (as for example some node modules do)
  • a client usually starts more than one server. If every server runs its own file system watching it can become a CPU or memory problem.
  • in general there are more server than client implementations. So this problem is better solved on the client side.

@pepeiborra
Copy link
Collaborator Author

Can I get a stamp please?

@wz1000 wz1000 closed this Mar 6, 2021
@wz1000 wz1000 reopened this Mar 6, 2021
@wz1000 wz1000 added the merge me Label to trigger pull request merge label Mar 6, 2021
@mergify mergify bot merged commit 1b245ca into master Mar 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge me Label to trigger pull request merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace GetModificationTime with a file version scheme via WatchedFile events
3 participants