Fix LSP timeout behaviour when loading a large package of small modules - #23224
Fix LSP timeout behaviour when loading a large package of small modules#23224cgibbard wants to merge 2 commits into
Conversation
The timeout gets reset any time that the server produces output, which fixes the case where damlc is making good progress on building modules but doesn't get through them all before hitting the timeout. Additionally, add configuration for the keepalive interval and timeout.
samuel-williams-da
left a comment
samuel-williams-da
left a comment
There was a problem hiding this comment.
Since you're allowing the timeouts to be changed, we might want to communicate the keep alive interval to the language server, via startup params. If you've reduced this 60s KA timer locally, have you faced any issues during inactivity?
| if (proc && typeof proc.kill === "function") { | ||
| proc.kill("SIGTERM"); | ||
| } |
There was a problem hiding this comment.
Killing processes here is tricky, since we support many platforms. Which platform have you tried this on? Are you able to verify this works in Windows?
There was a problem hiding this comment.
Well, this is what it was already doing to try to kill the process, but you're right that it's suspicious and we should check this. I don't have a Windows machine to hand, but I'll get one of my coworkers to try it.
There was a problem hiding this comment.
https://nodejs.org/api/child_process.html#subprocesskillsignal seems to indicate that on Windows, the kill method will kill the process forcefully when given any of 'SIGKILL', 'SIGTERM', 'SIGINT' or 'SIGQUIT', which perhaps is less nice than it ought to be (might not give the murdered process a chance to clean up subprocesses...), but I'm not certain yet what to do about that, if anything.
For fun, I also tracked down where this actually happens. Node delegates to the libuv library to abstract over OS-specific things, and ultimately ends up in uv__kill, here
I discovered during further experimentation. * Fixes a missing field error that was present from before: _childProcess was renamed to _serverProcess in the vscode-languageclient library, and if we actually tried to kill the server, we'd instead just error out due to the missing field * Also reattach the stdout/stderr listeners when the server process is restarted * Fix an issue where self.startKeepAliveWatchdog was being run with `this` set to the wrong object (or maybe undefined) when restarting it 10s after killing the process.
I did a bunch of additional testing here, and while I didn't have any issues reducing the intervals, I did actually find a few more issues, and fixed them. The new liveness checking had prevented the code path where we actually try to kill the server from being exercised at all, so I hadn't noticed some other problems with it. Upon sending a STOP signal to the running damlc so that it wouldn't respond to the keepalive, I noticed that the client died while trying to kill it. The culprit was this commit in 2017 to vscode-languageclient which renamed _childProcess to _serverProcess. (The Haskeller in me is screaming about how this ought to have been a static type error.) I also noticed once I fixed that that when the server came back up, after a 10 second timeout when we tried to restart the watchdog, the Following that, I realized that when the server restarts, our liveness event handlers would no longer be bound to the right process, so I added some code to reconnect them when the client's state changes. Doing a little more research on the timeout values point, on the server-side, both in the single IDE code and the multi IDE code the language server handles the keepalive by responding immediately with no apparent sensitivity to whatever timeouts the client is using. |
2 participants
These changes make the LSP client timeout get reset any time that the LSP server produces output, which fixes the case where damlc is making good progress on building modules but doesn't get through them all before hitting the timeout.
Additionally, this adds configuration options for the keepalive interval and timeout, with the same defaults as the current client had. There was a comment from (JM) to remember to update the language server's timeouts if the client's timeouts were changed, which seemed relevant at first (if we make it configurable, both things should use the same config), but I didn't actually see any code that depended on similar values over in daml-ide or daml-ide-core. If I was looking in the wrong place (or suffering from blindness 😅), please help me out. Happy to split the PR or drop the configurability if it turns out to be undesirable. The timeout resets more robustly fixed the issue we were having regardless.