fix(server): Terminate on service panic

[jjbayer]

Terminate the process when a panic occurs in one of the services.

Instead of implementing a sync fn spawn_handler() interface, each service now implements an async fn run() interface. This simplifies service implementations in most cases (except where services need to spawn multiple tasks), and allows us to join on all service main tasks to detect a panic.

The PR introduces a ServiceRunner utility to easily start & join on running services.

See also #4026 for an earlier attempt.

Note: The issue requires reporting "unhealthy" instead of terminating. I will take care of that in a follow-up PR.

ref: #4037

[jjbayer]

I confirmed that this terminates the process by inserting an explicit panic!() into one of the service bodies.

[Dav1dde]

Mid-/Long-Term it would be nice if a service can specify it's desired behaviour, abort on panic/restart on panic.

[loewenheim]

If my understanding is correct, this essentially separates the concern of how a service is spawned out of the trait by replacing the spawn_handler method, which contained a tokio::spawn call, with a run method which is just an async function. Then you can start the service using either a ServiceRunner which keeps tabs on the services under its control—specifically, whether they panic—or just plain tokio::spawn (in the form of the start_detached method).

My one gripe with this is one of uniformity. In many places, services are started with runner.start(service), but e.g. ProjectCacheService::start takes the runner as a parameter. It might be nicer to do that everywhere, i.e. have a start method on Service which takes a runner as a parameter, but it's ultimately a sort of aesthetic concern.

[loewenheim]

I'm a bit leery of the naming, start vs spawn isn't a very clear distinction. Can't think of anything better off the top of my head either, though.

[jjbayer]

How about start and start_with?

[loewenheim]

IMO something like start_with_rx or start_with_receiver would be even better.

[jjbayer]

If my understanding is correct, this essentially separates the concern of how a service is spawned out of the trait by replacing the spawn_handler method, which contained a tokio::spawn call, with a run method which is just an async function. Then you can start the service using either a ServiceRunner which keeps tabs on the services under its control—specifically, whether they panic—or just plain tokio::spawn (in the form of the start_detached method).

Yes!

My one gripe with this is one of uniformity. In many places, services are started with runner.start(service), but e.g. ProjectCacheService::start takes the runner as a parameter. It might be nicer to do that everywhere, i.e. have a start method on Service which takes a runner as a parameter, but it's ultimately a sort of aesthetic concern.

@loewenheim this bugs me too, I will unify it to * Service::start(self, runner: &mut ServiceRunner)

[jjbayer]

In many places, services are started with runner.start(service), but e.g. ProjectCacheService::start takes the runner as a parameter. It might be nicer to do that everywhere, i.e. have a start method on Service which takes a runner as a parameter, but it's ultimately a sort of aesthetic concern.

@loewenheim this bugs me too, I will unify it to * Service::start(self, runner: &mut ServiceRunner)

After trying this out, I figured it's actually better to keep the interface of Service narrow and only give the types that implement Service a helper method when they need it. I know consistently named these helpers start_in to clarify that they invert control.

classDiagram
    class ServiceRunner {
        +start(service)
    }

    ServiceRunner --> "starts" Service
    
    <<Interface>> Service
    class Service {
        async +run()
    }

    class ServiceWithStartHelper {
        +start_in(runner)
    }

    Service "implements" <|-- ServiceWithStartHelper

    ServiceWithStartHelper --> "calls" ServiceRunner

Loading

[Dav1dde]

Tokio has a JoinSet for awaiting/spawning tasks, can use that instead of FuturesUnordered.

[Dav1dde]

Mid-/Long-Term it would be nice if a service can specify it's desired behaviour, abort on panic/restart on panic.