Properly clean up timer in stop()

[Marenz]

No description provided.

[Copilot]

Pull Request Overview

This PR introduces a new override for the stop() method in the background service to ensure that a timer is properly stopped before proceeding with the shutdown process.

• Introduces an overridden stop() method that stops the _next_event_timer
• Calls the parent class's stop() method after stopping the timer

Comments suppressed due to low confidence (1)

src/frequenz/dispatch/_bg_service.py:223

• [nitpick] Consider expanding the docstring for the stop() method to clarify that stopping _next_event_timer is crucial for preventing potential resource leaks during shutdown.

@override

[Marenz]

Interesting that the CI fails when I add the stop method (one test)...

[llucax]

We are probably going to deprecate .stop() as it was something specific for Timer, but now we have close() in the Receiver interface, so it might be more future-proof to use close().

Also since this is a background service, it is better to put the close() in cancel() instead of stop(), otherwise if you d.cancel(); await d the timer won't be closed.

Finally this might be more complicated than you thought, at least we had issues in some other places where now you start getting a ReceiverStoppedError/StopAsyncIteration exception in the run you might need to start catching. Maybe this is a more actor-specific issue because of the auto-restart and is not really an issue for pure background services though, but just something to have in mind.

[Marenz]

Changed to close() and added reset() in start to fix failing test and also because it's correct

[llucax]

I'm starting to think what's really more correct is:

    async def _run(self) -> None:
        with closing(Timer(
            timedelta(seconds=100), SkipMissedAndResync()
        ) as next_event_timer:
            ...

This way you don't need to mess with start/stop/cancel, you don't need auto_start=False, and you have guaranteed cleanup. Do you see any problems with this?

BTW, probably the timer's period should be a constructor argument with 100 seconds just as a default.

[Marenz]

BTW, probably the timer's period should be a constructor argument with 100 seconds just as a default.

Why? It's overwritten in update_timer() for pretty much any activity

[llucax]

We talked already via DM but just for the records, it is basically to ensure proper cleanup, and to tie the lifetime of the timer to the lifetime of the task/run. Maybe is not worth it as we have other instance variables that would also make sense to move to run, but that is too much work and if only do it for the timer, we might end up in the middle of 2 worlds, so I leave it up to you.

[llucax]

Not approving because of the comment and auto-merge but feel free to self-merge if you address it.

[llucax]

self.is_running?

[Marenz]

I swear, I was looking for it