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.

Sign up for GitHub

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

Jump to bottom

Stop doing action for handles and ignore responses after shutdown #567

Merged
merged 20 commits into from
Apr 10, 2024
Merged

Stop doing action for handles and ignore responses after shutdown #567

Changes from 5 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
e63e1e0
stop doing action before handler after shutdown
michaelpj Apr 6, 2024
a1b7bb0
allowed method can run the before-action
soulomoon Apr 8, 2024
b6d5b04
fix
soulomoon Apr 8, 2024
cd35f7e
add log
soulomoon Apr 8, 2024
2066243
swap log location
soulomoon Apr 8, 2024
fcc1d06
block the response call back
soulomoon Apr 9, 2024
95e4b0b
stop tryChangeConfig
soulomoon Apr 9, 2024
7c5febc
Update lsp/src/Language/LSP/Server/Processing.hs
soulomoon Apr 10, 2024
c18a370
revert barrior in tryChangeConfig
soulomoon Apr 10, 2024
41d9e21
fix typo
soulomoon Apr 10, 2024
23cd222
revert tryChangeConfig
soulomoon Apr 10, 2024
fda5f24
add documentation
soulomoon Apr 10, 2024
3913962
Update lsp/src/Language/LSP/Server/Processing.hs
soulomoon Apr 10, 2024
03240c6
refine shutdown blocking
soulomoon Apr 10, 2024
49ec6f4
refine shutdown blocking
soulomoon Apr 10, 2024
e9efbc9
fix format
soulomoon Apr 10, 2024
a438b4b
fix format
soulomoon Apr 10, 2024
d933651
fix doc
soulomoon Apr 10, 2024
fc18852
fix doc
soulomoon Apr 10, 2024
ac343e9
Update Processing.hs
soulomoon Apr 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 10 additions & 8 deletions lsp/src/Language/LSP/Server/Processing.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ data LspProcessingLog
| forall m. MessageDuringShutdown (SClientMethod m)
| ShuttingDown
| Exiting
| LspMessage String

deriving instance Show LspProcessingLog

Expand All @@ -88,6 +89,7 @@ instance Pretty LspProcessingLog where
pretty (MessageDuringShutdown m) = "LSP: received message during shutdown:" <+> pretty m
pretty ShuttingDown = "LSP: received shutdown"
pretty Exiting = "LSP: received exit"
pretty (LspMessage s) = "LSP Msg:" <+> pretty s

processMessage :: (m ~ LspM config) => LogAction m (WithSeverity LspProcessingLog) -> BSL.ByteString -> m ()
processMessage logger jsonStr = do
Expand Down Expand Up @@ -449,31 +451,31 @@ handle' ::
TClientMessage meth ->
m ()
handle' logger mAction m msg = do
maybe (return ()) (\f -> f msg) mAction
shutdown <- isShuttingDown
let allowedMethod m = case (splitClientMethod m, m) of
soulomoon marked this conversation as resolved.
Show resolved Hide resolved
(IsClientNot, SMethod_Exit) -> True
(IsClientReq, SMethod_Shutdown) -> True
_ -> False

logger <& LspMessage (show m) `WithSeverity` Debug
when (not shutdown || allowedMethod m) $ maybe (return ()) (\f -> f msg) mAction
Copy link
Collaborator

Choose a reason for hiding this comment

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

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 :)

Copy link
Collaborator Author

@soulomoon soulomoon Apr 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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).🤔

Copy link
Collaborator Author

@soulomoon soulomoon Apr 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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:

  1. didChangeConfiguration sent to server
  2. workspace/configuration sent to client
  3. Response of configuration sent to server, we update the config.

What we are doing here is kind of mixed:

  1. didChangeConfiguration sent to server, we tryChangeConfig.
  2. workspace/configuration sent to client
  3. Response of configuration sent to server, tryChangeConfig again.

soulomoon marked this conversation as resolved.
Show resolved Hide resolved

dynReqHandlers <- getsState resRegistrationsReq
dynNotHandlers <- getsState resRegistrationsNot

env <- getLspEnv
let Handlers{reqHandlers, notHandlers} = resHandlers env
shutdown <- isShuttingDown

case splitClientMethod m of
-- See Note [Shutdown]
IsClientNot | shutdown, not (allowedMethod m) -> notificationDuringShutdown
where
allowedMethod SMethod_Exit = True
allowedMethod _ = False
IsClientNot -> case pickHandler dynNotHandlers notHandlers of
Just h -> liftIO $ h msg
Nothing
| SMethod_Exit <- m -> exitNotificationHandler logger msg
| otherwise -> missingNotificationHandler
-- See Note [Shutdown]
IsClientReq | shutdown, not (allowedMethod m) -> requestDuringShutdown msg
where
allowedMethod SMethod_Shutdown = True
allowedMethod _ = False
IsClientReq -> case pickHandler dynReqHandlers reqHandlers of
Just h -> liftIO $ h msg (runLspT env . sendResponse msg)
Nothing
Expand Down
Loading