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.

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

Refactor Workerqueue, add waiting empty utility for testing #4296

Open
wants to merge 18 commits into
base: master
Choose a base branch
from

Conversation

soulomoon
Copy link
Collaborator

@soulomoon soulomoon commented Jun 8, 2024

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

  • add new data type WorkerQueue that wrap either TMVar or TQueue.
  • add waitUntilWorkerQueueEmpty for the test.
  • Peek the action from the queue, run it and then remove it from the queue.
  • add withWorkerQueueOfOne to use TMVar version of the queue aka queue size of 1.

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 soulomoon force-pushed the soulomoon/make-workerqueue-cancellable-if-not-started branch from 5745a7d to 06e3b8f Compare June 8, 2024 19:33
@soulomoon soulomoon added the performance Issues about memory consumption, responsiveness, etc. label Jun 8, 2024
@soulomoon soulomoon requested a review from michaelpj June 8, 2024 19:37
@soulomoon soulomoon changed the title Make workerqueue cancellable if not started already configurable Make workerqueue action cancellable if not started already configurable Jun 8, 2024
@soulomoon
Copy link
Collaborator Author

soulomoon commented Jun 9, 2024

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

@michaelpj
Copy link
Collaborator

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
Copy link
Collaborator Author

soulomoon commented Jun 9, 2024

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
Copy link
Collaborator Author

soulomoon commented Jun 10, 2024

@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.

  • An action is only removed from the queue after it is done running.
  • add waitUntilWorkerQueueEmpty for the test

Fix #4309 part of #4300

@soulomoon soulomoon changed the title Make workerqueue action cancellable if not started already configurable Refactor Workerqueue, add waiting empty utility for testing Jun 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Issues about memory consumption, responsiveness, etc.
Projects
None yet
2 participants