Avoid race conditions with VFS and VFS versions

[wz1000]

We need to take VFS snapshots as soon as we get a change notification.

Consider the following interleaving of events:

1. Change Notification A (updates LSP VFS)
2. Restart Shake Session (A changed) initiated
3. Change Notification B (updates LSP VFS)
4. Restart Shake Session (A changed) takes VFS snapshot and possibly performs more computation
5. Restart Shake Session (B changed)

In particular, between step 3 and 5, we took a snapshot for a previous build,
but this snapshot included changes from a newer VFS state that the build should
not have seen.

To fix this, we need to take snapshots as soon as a notification handler is
called, before forking any threads. This works because LSP calls all handlers
in a single threaded fashion and these handlers block message processing. It
is essential to do this on the LSP handler thread rather than the reactor thread
that GHCIDE sets up in order to maintin the property.

[pepeiborra]

Nice

[fendor]

Awesome, thank you!

[michaelpj]

I'm wondering what we could do to make this easier. Would it be helpful to have the ability to install a callback which gets called whenever the VFS gets modified?

[wz1000]

The DidChange etc LSP notification handlers are the callback.

[michaelpj]

The DidChange etc LSP notification handlers are the callback.

Sure, but you have to know that, and you have to deal with all of them. You can't just say "please tell me when the VFS changes". So we're failing to provide a usable abstraction!

[wz1000]

Earlier on IRC I suggested we could pass the VFS value along with the notification handlers that cause it to be changed, and this could be extended to the other kinds of state that lsp manages, including sending changed configuration values on configuration change events etc.

Alternatively, I don't think it would be too bad of an idea to remove this state on the lsp side entirely, relying on users to maintain and update their own VFS data using functions provided by lsp.

[michaelpj]

Earlier on IRC I suggested we could pass the VFS value along with the notification handlers that cause it to be changed, and this could be extended to the other kinds of state that lsp manages, including sending changed configuration values on configuration change events etc.

My problem with this is that handlers already run in LspM which is supposed to exactly provide you with access to these things! So it's quite confusing to provide it in two ways. And it rather suggests that the approach getting it via LspM isn't fit for purpose.

Alternatively, I don't think it would be too bad of an idea to remove this state on the lsp side entirely, relying on users to maintain and update their own VFS data using functions provided by lsp.

I feel like you're just going to end up saying that lsp shouldn't provide a server implementation at all. That might be right! Maybe it shouldn't. We could just provide the pieces.

---

Okay, one more idea: what if we had a "big lock" on the LSP server state. You could take the big lock if you're going to do something long-running that relies on having a consistent view of the state. The cost would be that it would block anything else from progressing, but that might just be fine.