Skip to content

Conversation

@victorges
Copy link
Member

@victorges victorges commented Dec 20, 2024

After working on #371 I realized how messy the lifecycle management of the streamer was.
This is to simplify and improve that a bunch.

Issues

Previously, we started a bunch of loose background tasks and had no central way of controlling them.
There were mainly 2 issues:

  • Some of the tasks were started/restarted together with the inference process. This caused an additional complexity in the tasks themselves and in the task management logic.
  • The stop() function was called from several different places, including from the tasks themselves that were being restarted. The restart function also killed some of the tasks that called it (cause they were recreated with the process).

Fixes

So this improved this 2 issues in 2 steps:

  • First made sure that all the asyncio tasks are started with the streamer itself. The main change here was changing the looping logic so we check the main "stop_event" instead of the process.done() event.
  • Second, created a new "supervisor task" to rule them all. Instead of calling stop from multiple places, only the supervisor task actually stops everything now anytime one of the tasks end or the stop_event is set.

This will also make sure that we don't keep the streamer running when any of the tasks fail. This happened for the monitor loop on a recent version and no one ever realized, cause it only got logged but didn't crash the whole thing.

@victorges victorges force-pushed the vg/fix/streamer-lifecycle branch from 7c68cf3 to 66ba42d Compare December 20, 2024 22:41
This was actually broke since I introduced the status
but it was actually never tested :3

Fixed now!
@victorges victorges merged commit 0ce510d into main Dec 20, 2024
10 of 11 checks passed
@victorges victorges deleted the vg/fix/streamer-lifecycle branch December 20, 2024 23:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants