Replace Service's future impl by not-self-consuming method

[cecton]

Problem it tries to fix

When the Service needs to be shutdown, run_service_until_exit does:

1. extracts the telemetry and the base_path from the Service
2. drop the Service
3. drop the tokio runtime which will wait for all the tasks to finish

The reason why we do this in this particular order is because dropping the service will trigger the drop of the task_manager which will trigger the task's exit.

But this is annoying because it means we need to keep references on things that we don't want to be deleted during the tear down (telemetry and base_path).

A better approach would be to:

1. signal termination (all tasks must finish)
2. drop the tokio runtime

This would make sure that the things that must not be dropped before the tokio runtime are kept.

Proposed solution

1. add a terminate() method on the task_manager and on the Service to signal all the task to finish
2. add a future() method on Service that will not consume self, thus not drop the service when the future is dropped
3. remove the Future impl on Service as you most likely don't want to lose your Service after a failure

I think the point 2. & 3. are not required but it makes more sense for the API.

[parity-cla-bot]

It looks like @cecton signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

[cecton]

tbh I'm not sure why those tasks are spawn because the Future impl Service is just waiting indefinitely and stops if there is an essential task failing. But since this is spawn like a background process, nobody is doing anything with the result anyway. Will see if it breaks on the CI

[tomaka]

The Service used to properly own the tasks that it runs, and process them manually if no executor is available, rather than always spawning these tasks in the background.

[cecton]

That explains everything

[tomaka]

I guess this is all debatable, but it was initially designed this way because good programming practices consist in stopping or killing stuff when destructors are being run, rather than having to call a terminate method.

[tomaka]

In my opinion we have the choice between two approaches: either make the Service properly own everything it runs and properly hide implementation details, or turn it into a dummy struct with public fields which acts as the success value of a building function that spawns a bunch of tasks.

Having a Service whose constructor spawns tasks, but which can then be destroyed without these tasks stopping, is a middle ground that doesn't really make sense to me.

[tomaka]

I don't really have a clear opinion on this PR or how the Service should be designed in particular, but I'm of the opinion that we should have a clear design to follow rather than making incremental changes that seem to go a bit in all directions.

[cecton]

🤔 it makes a lot more sense to me now.

I guess in the current evolution of the Service it would be logical to make the Service more dumb. The alternative to this would be for the Service to own the tokio runtime so it can stop the tasks properly when dropping. But I suppose that is something we don't want as it would limit how substrate can be used.

[cecton]

Fixed by ec2ab79