Stop doing action for handles and ignore responses after shutdown #567
Conversation
soulomoon
force-pushed
the
stop-doing-action-before-handle-after-shutdown
branch
from
f4cb6df
to
e63e1e0
Compare
| _ -> False | ||
|
|
||
| logger <& LspMessage (show m) `WithSeverity` Debug | ||
| when (not shutdown || allowedMethod m) $ maybe (return ()) (\f -> f msg) mAction |
There was a problem hiding this comment.
I think this would be stronger than it needs to be. In particular, it's not a problem if we e.g. update the VFS. It's really only the config callback that's potentially problematic.
It's also not enough, since updating the configuration is complicated. This handles the didChangeConfiguration notification, but not the response to workspace/configuration. So if we wanted to do this, I think we should just stick the check in tryUpdateConfig or something.
And I still think that servers should just make their config callbacks safe :)
There was a problem hiding this comment.
You are right , it is not enough to stop the callback.
But IMO I do not see any reason to do the action before noti/req even it is safe to do so, since the server is marked as shutdown anyway(Waitting the cleanup and exit notification, and ignoring other notification and request, it seems reasonable to ignore thoese actions bring up by recieving the notification and request after the shutdown).🤔
There was a problem hiding this comment.
After reading the relevant issues, Is my understanding correct about config update?
There is push model and pull model(new one) in LSP,
The pull model:
didChangeConfigurationsent to serverworkspace/configurationsent to client- Response of configuration sent to server, we update the config.
What we are doing here is kind of mixed:
didChangeConfigurationsent to server, wetryChangeConfig.workspace/configurationsent to client- Response of configuration sent to server,
tryChangeConfigagain.
lsp/src/Language/LSP/Server/Core.hs
Outdated
| logger <& NewConfig newConfigObject `WithSeverity` Debug | ||
| cb <- LspT $ asks resOnConfigChange | ||
| liftIO $ cb newConfig | ||
| shutDown <- isShuttingDown |
There was a problem hiding this comment.
so instead of stop callback to all responses after we shutdown, we are stopping the config update callback here?
There was a problem hiding this comment.
You convinced me, let's not do this and instead skip both responses and actions like you suggest.
|
I think it is pretty counter intuitive that we are reponding with a invalid request but still do the actions. |
That's fair. What I am planning to do at some point is to entirely move all this stuff to be just default handlers for these methods. Then they would be treated uniformly. I guess also that not accepting responses is pretty reasonable. We might well expect that clients won't send responses if they've send |
Co-authored-by: Michael Peyton Jones <me@michaelpj.com>
Co-authored-by: Michael Peyton Jones <me@michaelpj.com>
Agree, this should make things better. |
|
Need to fix formatting |
soulomoon
changed the title
fixed now |
haskell/haskell-language-server#4153