Skip to content

Add Gitlab::BackgroundTask to supersede Gitlab::Daemon

Matthias Käppler requested to merge 367861-composable-daemon into master

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 sleeps. 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 a state ivar that can be idle, running or stopped.
  • The Daemon would swallow errors in stop, which obscured problems occurring on background threads. These are now properly logged via ErrorTracking.
  • 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.

Related to #367861 (closed)

Edited by Matthias Käppler

Merge request reports