search

lukel97 / lsp-test

A functional test framework for LSP servers
BSD 3-Clause "New" or "Revised" License
35 stars 15 forks source link

Kill timeout thread to prevent TimeoutMessages from clogging up the queue #88

Closed wz1000 closed 3 years ago

wz1000 commented 3 years ago

Also added some strictness annotations and switched an MVar to an IORef.

This greatly improves the performance of the benchmark added in https://github.com/alanz/lsp/pull/287

jneira commented 3 years ago

I would like to test this locally in windows with the ghcide/hls test suite, windows is more sensitive to thread kills than linux

wz1000 commented 3 years ago

Sure, but I can't imagine anything really going wrong. This thread is only responsible for sending a timeout message to the queue (and this message is ignored once the action finishes). Are you saying killThread causes crashes on windows? This would be very surprising, since it means that async exceptions are completely broken on windows, and ghcide uses async exceptions to cancel threads all the time (like whenever the user types something, it cancels any running compiles)

jneira commented 3 years ago

Mmm, thanks for the explanation, sure i am overlooking it, last time there were problems with thread kills were due to opened handles iirc.

wz1000 commented 3 years ago

This thread opens no handles, it just sleeps for a while and then writes a message into a channel.

jneira commented 3 years ago

The ci was failing in the build phase, no?

lukel97 commented 3 years ago

@jneira that CI failure was caused by lsp[-types] not having an upper bound on lens, it was happening across all the PRs. I made a new revision of lsp-types that pins lens to < 5 and it seems to be working now! alanz/lsp/1778cabf168f9c1e8235cb273bdba85eedba5813 explicitly adds DataKinds extension into Lens.hs though which fixes it when building with lens >= 5, so we don't need to actually change anything in lsp.cabal

jneira commented 3 years ago

nice, thanks for fixing it and the info