haskell / haskell-language-server

Official haskell ide support via language server (LSP). Successor of ghcide & haskell-ide-engine.
Apache License 2.0
2.61k stars 351 forks source link

Refactor Workerqueue, add waiting empty utility for testing #4296

Open soulomoon opened 2 weeks ago

soulomoon commented 2 weeks ago

Follow up of #4256 See also #4297 It also fix #4309 What's done:

SessionLoader can make use of withWorkerQueueOfOne, so the load action won't be enqueue until the last one is done. It help to remove the duplication runs of SessionLoader when sessionRestart is called at the time the loader try to enqueue a load action but the old load action is still running. It make the scheduling of loading critical session cancellable

soulomoon commented 2 weeks ago

cc @michaelpj , take a look at this follow up. it should improve the semantic of the workerQueue

michaelpj commented 2 weeks ago

Okay, now I really am suspicious. Session loading uses awaitRunInQueue and the queue can only have one thing in it. That really is just a lock! I think it might just make sense to use a different mechanism there, and keep the worker queue for cases where we really do have a queue of things we want to do asynchronously.

I'm also sceptical about the restart queue. Are we really going to have multiple restarts queued up? And if that can be seen as just needing to take a lock, well, that's already there in the form of the withMVar call.

soulomoon commented 2 weeks ago

restart queue should queue up, since we do not want the restart to get lost even if the thread waiting to do it is cancelled. Or broken build system state might happen.

Session loading is a different case, since it is called by the rule system. The rule system would handle the restart case.

But we do want them to run in a separate thread.

  1. Session loading need to run in separate thread one way or another since the thread(under hls-graph manage) calling it might be cancelled anytime. Exit session loading in the middle might cause broken state.
  2. Both session loading and restart need to be run in a separate thread to handle the shutdown correctly.

The point is less about the queueing but rather a separate thread for these critical session to run and the thread can be exit if we need to shutdown.

soulomoon commented 2 weeks ago

@michaelpj , I refactored this PR to shift the semantic of our workQueue to run then remove. Then we can monitor and wait for the queue to be empty and ensuring no critical action is running. This is the part of waiting utility we need to remove the unconditional wait.

Take another look. I think it also add one more justification to our putting the session loading and shake restart into workQueue. We can better monitor the running status.

Two more thing have been done.

Fix #4309 part of #4300