Support adding services while ServiceGroup is running

[sliemeobn]

somewhat straight-forward implementation draft of
#185

choices were made, what do we think?

[FranzBusch]

This is a great start. Thanks for working on this. I left some feedback inline.

[sliemeobn]

ok, merging the addService and the taskGroups signals into the main run loop is a lot tougher than expected...
I think I'd need something like a non-sendable, non-escaping merged iterator that can handle this.

but the async algorithms package only has a sendable-required merge function.

how off am I? any better ideas?

[FranzBusch]

ok, merging the addService and the taskGroups signals into the main run loop is a lot tougher than expected... I think I'd need something like a non-sendable, non-escaping merged iterator that can handle this.

but the async algorithms package only has a sendable-required merge function.

how off am I? any better ideas?

Yeah I expected you run into this. So the problem is that TaskGroup is not Sendable because it can't be iterated concurrently. What we could do is send the iterator of the async channel into a child task, then consume the next service once, return it from that child task similar to how all other child tasks are returning one case of an enum. Then we add the service and spawn another task consuming off the next service that needs to be added.

This way we only have to transfer the iterator of the channel in and out of child tasks which requires some unchecked sendable but is totally safe. Does that make sense?

[sliemeobn]

^^ I was just coding this exact thing, but I wasn't sure if it is too crazy ; )
I'll see how it goes...

[sliemeobn]

@FranzBusch I managed to clean up the channel reading, I think that part is good enough now.

The remaining question is what to do with added services during shutdown or cancel?

If we want a reliable back-propagation of "has it run or not?" we'll need to pass along a continuation I think and we'd need to make sure to fully drain and actively "reject" all added service until we set the status to .finished.

A lot of brittle and racy signaling... if we simply ignored (like now) life would be a lot easier ; )

[sliemeobn]

thanks @FranzBusch for the quick reviews!

I addressed the feedback from above, but I am still a bit scared of the "best effort" nature of it.

So, as a suggestion, I renamed the function to addServiceUnlessShutdown to better communicate its best-effort nature (similar to TaskGroup.addTaskUnlessCanceled).

This would also allow us to one day add a addService that maybe always runs and immediately shuts down, and maybe throws if the group was canceled or has shutdown. (or similar, should the need arise)

wdyt?

[FranzBusch]

This looks great. I'm happy with the new name and the implementation looks good to me. Just one last nit around the comment

[FranzBusch]

@sliemeobn can you fix the license header check please

[sliemeobn]

can you fix the license header check please

@FranzBusch oops, done

(is there an easy way to run the soundness checks locally?)

[FranzBusch]

can you fix the license header check please

@FranzBusch oops, done

(is there an easy way to run the soundness checks locally?)

Yeah you can use act to run all of it locally. There is some documentation here: https://github.com/swiftlang/github-workflows?tab=readme-ov-file#running-workflows-locally

[czechboy0]

Apologies for the late review - it'd be great to call out that this can also be used for adding services before the group was started. Right now the comment leaves that ambiguous of what happens.