https://bugs.kde.org/show_bug.cgi?id=438790
--- Comment #1 from nyanpasu64 <nyanpas...@tuta.io> --- I investigated the LSP client. Pressing Ctrl+C in gdb during the hang pointed within LSPClientServerManagerImpl::~LSPClientServerManagerImpl(). The corresponding function as of today's master is https://github.com/KDE/kate/blob/45362eaa64ede7b0dbb172a5c154859c76fd6231/addons/lspclient/lspclientservermanager.cpp#L213, which matches my local 21.04.2 function. According to the source, Kate's LSP client is designed to sleep the UI thread for 500ms (using QThread::msleep(500)) after sending a termination request to LSPs, before sending SIGTERM and SIGKILL (each with a 100ms delay afterwards). I'm not sure if waiting for language servers to terminate is a bad design, or if blocking the UI thread is a bad design, or what would be better. However, the current design is implemented poorly, since it sleeps for 700ms even though `m_servers` is empty (for example, in an empty Kate window without open files)! Also there's some oddities going on, with `connect(s.data(), &LSPClientServer::stateChanged, this, handler)`, a `QTimer t` that never fires, and a `QEventLoop q` that seems to go unused aside from calling quit() on it. I don't understand what `count` is doing either. Maybe it can be removed entirely (but I don't know what to do with `handler`). I think it's dead code that used to be active: https://github.com/KDE/kate/commit/724a9d0af0e9b4ec04d91f74ca9bbc9dbeaa2ee2 Additionally, the current code checks for `auto &s = si.server; if (!s)` to determine whether to request shutdown, SIGTERM, then SIGKILL. I don't think `ServerInfo { QSharedPointer<LSPClientServer> server` will become a null pointer when the LSPClientServer shuts down (but I didn't verify it doesn't get overwritten). Assuming it doesn't become a null pointer, that means Kate calls LSPClientServer::stop() (which calls QProcess::terminate()/kill()) even if the process is already dead! ---- An easy partial fix is to skip the m_servers/msleep() song and dance entirely if m_servers is empty. This will fix the shutdown lag if no language servers are running. If maintainers want to keep the current overall behavior (blocking the UI thread while sending terminate+SIGTERM+SIGKILL), I think a better fix is: - If any servers are active, send a termination request, "join" on them with a 500ms timeout, then remove the servers which have shutdown. - If any servers are left, send SIGTERM, "join" on them with a 100ms timeout, then remove the servers which have shutdown. - If any servers are left, send SIGKILL and don't bother joining anymore. If they don't shutdown from SIGKILL, there's nothing we can do (I think). If you feel like it, you can send the user a popup if LSPs don't respond to termination, SIGTERM, or SIGKILL. Issue is, I don't know how to properly implement "joining", and haven't researched how to do it yet. I can look into it if the maintainers like my proposed control flow. ---- While researching this problem, I was exploring the various Kate plugin APIs, and have some comments about the design. `LSPClientServer::stop(int to_term_ms, int to_kill_ms)` is unintuitive to me, but I don't think it can be significantly improved (aside from using a constant like DONT_TERMINATE instead of -1), since all combinations of {-1, >=0} for the two parameters are used (in ~LSPClientServerPrivate() and ~LSPClientServerManagerImpl(). I also found `LSPClientServerManagerImpl::restart(const ServerList &servers)` (https://github.com/KDE/kate/blob/45362eaa64ede7b0dbb172a5c154859c76fd6231/addons/lspclient/lspclientservermanager.cpp#L392). Instead of waiting for the old process to terminate, it sends a termination request immediately, SIGTERM in 0.4 seconds, SIGKILL in 0.8 seconds, and emits serverChanged() (which somehow respawns the server) in 1.2 seconds. This design is slower and arguably less elegant than waiting for the processes to terminate, but simpler and has more predictable behavior. You can watch the LSP servers stop and start by running `watch -n0.1 'pstree -a $(pgrep kate)'` and triggering LSP restarts in Kate. - Interestingly, if you restart the LSP, then press "jump to definition" before clangd starts up, it fails, but instantly spawns a clangd so pressing "jump to definition" a second time works. - By mapping a shortcut to "Restart All LSP Servers" and holding the shortcut down, you can get Kate to start close to 10 clangd processes at once. I don't know if this is a problem, or how to fix it. -- You are receiving this mail because: You are watching all bug changes.
