Add Gitlab::BackgroundTask to supersede Gitlab::Daemon
What does this MR do and why?
We run several background tasks in Puma and Sidekiq processes that are instances of Daemon
. This is an abstract base class currently that puts a particular workload on a background thread and takes care of orderly shutdown, handling interruptions etc.
It works quite well, but its design means that all implementations of it (subclasses like RubySampler
, servers etc.) are notoriously difficult to test. That's because its API interface is based on sub-typing and template methods that these subclasses need to implement, particularly run_thread
which executes inside the daemon thread.
Writing such tests means concurrency must be taken into account via condition variables or sleep
s. This makes tests flaky and hard to write.
This MR introduces BackgroundTask
, which tries to retain the good things from Daemon
while dropping the bad. Changes worth highlighting:
- The actual task implementation is isolated to a collaborator and merely dispatched to. This means we have a clear separation between scheduling concerns and domain logic. This will make the task logic much easier to test.
- The state a
Daemon
could be in was a bit murky and relied on nulling out references to the underlying thread. I made states more explicit by modeling it through astate
ivar that can beidle
,running
orstopped
. - The
Daemon
would swallow errors instop
, which obscured problems occurring on background threads. These are now properly logged viaErrorTracking
. - I removed the support for singleton instances. This does not make sense anymore, because task logic and
BackgroundTask
are instantiated separately now. It is up to the caller to handle the life cycle of these objects.
This class is currently unused. I have several candidates in mind, including ongoing work, which could utilize this. We should do this incrementally though and move code that uses Gitlab::Daemon
over one by one. Here is an example of an existing class being migrated to BackgroundTask
: !92885 (merged)
Screenshots or screen recordings
These are strongly recommended to assist reviewers and reduce the time to merge your change.
How to set up and validate locally
Numbered steps to set up and validate the change are strongly suggested.
MR acceptance checklist
This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.
-
I have evaluated the MR acceptance checklist for this MR.
Related to #367861 (closed)