Allow task manager to have children

[cecton]

Register another TaskManager to terminate and gracefully shutdown when the parent terminates and gracefully shutdown. Also ends the parent future() if a child's essential task fails.

This is related to paritytech/cumulus#164

[bkchr]

Why a box?

[cecton]

🤔 fixed 😁

[bkchr]

This optimization is not required.

[cecton]

it is not XD select_all panic if the list is empty https://docs.rs/futures/0.3.5/futures/future/fn.select_all.html

join_all doesn't though 🤔

[bkchr]

Why is this whole function not async? Instead of returning a boxed Future?

[cecton]

Fair point!! I think I'm used to do that because you can have async fn in traits.

But it still doesn't work:

error[E0733]: recursion in an `async fn` requires boxing
   --> client/service/src/task_manager/mod.rs:288:40
    |
288 |     pub async fn clean_shutdown(mut self) {
    |                                           ^ recursive `async fn`
    |
    = note: a recursive `async fn` must be 

I isolated the suspect. It seems those 2 lines make the function "recursive":

		join_all(children_shutdowns).await;
		completion_future.await;

[bkchr]

Did you remove the internal async move?

[cecton]

yes obviously XD

[cecton]

@bkchr I do agree that your change is more concise but "nicer"....? it's really less readable imo.

Maybe you didn't like the select! in the loop in my implementation but this kinda basic example of the doc.

On the other hand, I believe I have well tested, if the tests pass it definitely is equivalent.

(I really don't mind keeping the change you did, I think there is a question of taste and subjectivity)

[cecton]

@bkchr how about this? 4fdf803

[bkchr]

Your logic is not correct.

You need to wait for all children and the main task manager. You exit if the main task manager is finished before.

I also don't understand why we exit early when an essential tasked ended? These tasks will end when bringing down the node and thus, we still could see that the node crashes.

[cecton]

You need to wait for all children and the main task manager. You exit if the main task manager is finished before.

Are you sure you are not forgetting about the graceful_shutdown? because this function future is not here to wait for the tasks to finish

I also don't understand why we exit early when an essential tasked ended? These tasks will end when bringing down the node and thus, we still could see that the node crashes.

I think you are forgetting that there is a call to graceful_shutdown that will wait for those essential tasks to finish cancelling

[cecton]

I also don't understand why we exit early when an essential tasked ended?

It doesn't exit when an essential task ended, it exits when an essential task fails only

These tasks will end when bringing down the node and thus, we still could see that the node crashes.

They end by cancellation (or they could even naturally enb) but they don't fail with a panic so they wont trigger the essential task shutdown.

[bkchr]

Yeah sorry, I read the diff wrong!

[cecton]

Why did I call that "add_children" if it adds only one child? XD