SE-0381: discardResults for TaskGroups

This behaviour is still on.

4 Likes

+1

Are there any production use cases where we would want to keep the results for void?

1 Like

I think it's reasonable to restrict the generic type parameter to Void.

6 Likes

How about adding an overload where Value == Never where the user can return any value they want?

Confirming two things:

  • Offering this to ChildTaskResult == Void sounds good to me; it's not like you can do anything with the results of child functions after all anyway.
  • This is still a TaskGroup, so it does all the things expected of it as a structured concurrency primitive. So yes, it always waits for all children to complete.
    • this is safely implementable, unlike a next() method -- so the group.waitForAll() still exists and works as expected.

And one note about a minor syntax error in the proposal that I noticed while implementing and that we'll address in the proposal text:

public func withThrowingTaskGroup<ChildTaskResult, GroupResult>(
  of childTaskResultType: ChildTaskResult.Type,
  returning returnType: GroupResult.Type = GroupResult.self,
  discardResults: Bool = false,
  body: (inout ThrowingTaskGroup<ChildTaskResult, Error>) async throws -> GroupResult
) async rethrows -> GroupResult {

this has to be throws since there never is a next() that causes the throw to happen, so it must be:

public func withThrowingTaskGroup<ChildTaskResult, GroupResult>(
  of childTaskResultType: ChildTaskResult.Type,
  returning returnType: GroupResult.Type = GroupResult.self,
  discardResults: Bool = false,
  body: (inout ThrowingTaskGroup<ChildTaskResult, Error>) async throws -> GroupResult
) async throws -> GroupResult {

cannot tie this throw-ness to "was the boolean true?".

Good catch. This also means we can no longer just add discardResults as a new parameter to the already existing function, but need to introduce a completely new function. It will also only make sense to call the new function with discardResults: true. With that, I'm wondering if it would make more sense to give this function just another name and remove the discardResults parameter as propose during the pitching phase by @hisekaldma. Otherwise it will be a bit confusing to see two very similar looking methods with quite different behaviour but only if the one of the parameters is true.

1 Like

It's a new function anyway because ABI.

It is a bit confusing with the bool flag yeah: regardless if true/false, the signature is throws, but only if true, would it actually throw (or if the body throws). The new type is still the cleanest approach but we've tried to steer away from that. Perhaps it's still worth revisiting, even if impl internally uses the same internals... We could perhaps nest it like TaskGroup<Void>.DiscardingResults or something, but we'd still need to duplicate a bunch of API surface... but not implementation since we could reuse the same impl internally.

I'm wondering if you have thoughts which direction to go here @Douglas_Gregor.

1 Like

To clarify, I'm not talking about adding a new type, just adding a new function and reusing the existing TaksGroup type. The difference between what is currently propose and my proposal is basically

- withTaskGroup(of:returning:discardResults:body:)
+ withDiscardingTaskGroup(of:returning:body:)
- withThrowingTaskGroup(of:returning:discardResults:body:)
+ withDiscardingThrowingTaskGroup(of:returning:body:)

keeping the types all the same but just removing discardResults parameter and instead adding Discarding to the method name. The amount of new API surface would be exactly the same as the current proposal i.e. two new functions. Note that in the internal implementation of TaskGroup we will likely still need to use a boolean flag of some kind.

4 Likes

Yeah, I was clarifying that either way it is a new method, not just adding a parameter, but yeah form an user's perspective that's different.

The new name could be good as it makes the signature strictly correct again, though it pushes us towards actually making a ResultDiscarding Task Group IMHO, it's a bit weird that we have a new method, and chance to do the right thing with not offering next() but don't do so. The good news is that the underlying C++ implementation would be shared, and no new builtins (well, one which we need anyway), would be added... making this much leaner than the initial "complete new type with complete new impl" approach we had early on.

We'll have to see what the language and concurrency team would prefer. Adding a new type for this was considered too heavy, but perhaps given all those things and a better name for it it may be on the table after all?

1 Like

If I’m following the discussion correctly and there’s agreement that the child task result type should be Void, and the group result type is ignored, shouldn’t this just be…

+ withDiscardingTaskGroup(body:)
+ withThrowingDiscardingTaskGroup(body:)

…which seems pretty nice, and overall an even more elegant solution than having a Boolean toggle.

Now, if it simply must be that we need new “variants” of TaskGroup for “discarding-ness” just like for “throwing-ness,” say for the purpose of eliminating nonsensical uses of next(), my personal two cents are that that’s fine.

It’s actually quite a different end result than the originally pitched idea of a distinct notion of a “task pool” type with its own separate hierarchy of “throwing-ness,” even if the end result is having additional types.

6 Likes

I agee, we should drop the of childTaskResultType: ChildTaskResult.Type parameter if we constrain it to Void anyway (which I think we should). FWIW Swift 6 will not even allow a same-type constraint in this case and the compiler already produces a warning today: Same-type requirement makes generic parameter 'ChildTaskResult' non-generic; this is an error in Swift 6.

I'm not sure if removing the returning returnType: GroupResult.Type = GroupResult.self parameter is a good idea though. It can still be useful to eventually return something from within the body function e.g. a count of created child tasks.

2 Likes

This is what I suggested at the end of the pitch thread. Given that the throwing version must be throws instead of rethrows, I think it makes even more sense. Writing it all out, it would be:

public func withDiscardingTaskGroup<GroupResult>(
  returning returnType: GroupResult.Type = GroupResult.self,
  body: (inout TaskGroup<Void>) async -> GroupResult
) async -> GroupResult {

public func withDiscardingThrowingTaskGroup<GroupResult>(
  returning returnType: GroupResult.Type = GroupResult.self,
  body: (inout ThrowingTaskGroup<Void, Error>) async throws -> GroupResult
) async throws -> GroupResult 

This design really seems much more intuitive to me. Having a type parameter that must always be used with the same type feels like an annoyance at best, and an attractive nuisance at worst.

3 Likes

The number of differences between an existing task group and this discarding-results-task group is larger than it appeared while this was discussed in the pitch phase, so it's worth revisiting the idea of a new type. Personally, I find the idea of calling it DiscardingTaskGroup to go along with withDiscardingTaskGroup much more appealing than the originally-proposed TaskPool. My main complaints at pitch phase were (1) almost complete overlap in functionality (which is less true now) and (2) introducing the "pool" name alongside "group".

As for API surface... there isn't a whole lot of API on this once it becomes its own type, so the duplication isn't that bad. And it doesn't seem like there's much utility in (say) adding a protocol to unify the two types, because I doubt anyone would want to write generic functions based on that protocol.

Doug

14 Likes

+1 I have been chatting with @ktoso about this today already where he explained me all the little differences in behavior between the discarding and non discarding version. My fear was that it will become very hard to reason about Code and teach these nuances to new people. A separate type should make this way clearer.

5 Likes

Can we have those differences comprehensively enumerated here?

1 Like

Apologies, I'm working on a comprehensive writeup about the things we learnt in course of this review and addressing the feedback in the implementation :pray: My chat with Franz was a bit of a rant while implementing the edge cases around cancellation/error handling :sweat_smile:

I'll have an update for this thread as well as the proposal ready soon enough, hopefully today already! Thank you very much for the input everyone, we're arriving at a much nicer end result I think.

Short version / preview:

  • new type sounds good after all, hooray
    • new method sounds good, the naming discussions have resolved the concerns we had about the new type - thank you everyone for the feedback
  • cancellation and error handling behavior is far more complex than we thought at first, since the usual group patterns don't work due to the disappearance of try await next()
    • cancelling "siblings" needs special handling since we cannot rely on the next() throwing out of the group body anymore
10 Likes

Hi everyone, sorry for the wait.

Here's an update to the proposal that explains the error handling things we've stumbled into, as well as adopting the with[Throwing]DiscardingTaskGroup methods/types: [SE-0381] discardResults becomes [Throwing]DiscardingTaskGroup by ktoso · Pull Request #1889 · apple/swift-evolution · GitHub

I believe we should be able to amend/extend this review round, as the overall consensus so far has been in that direction.

I'd be interested in feedback about the error handling and group cancellation aspect specifically, which is explained in the new "Error propagation and group cancellation" section in the updated proposal. We believe those are the right semantics, but would love to hear what people think the error thrown in this situation should be:

withThrowingDiscardingTaskGroup() { group in 
  group.addTask { try boom(1) }
  try boom(2)
}

The boom(1) and boom(2) are racing here, and it is probably the most consistent to keep close to the promise that the "first" error thrown will be propagated out of the group -- as we promise this in general already for child tasks.

We are not considering adding the "error reducer" functionality in this take though.

Paging @Douglas_Gregor for review manager duties, thanks in advance! :pray:

8 Likes

I would expect boom(2) to be thrown here, but only because I expect the outermost block here to run synchronously for lack of explicit await. In other words, I expect the order of operations to be:

  1. Task group is created
  2. try boom(1) is scheduled, but not run
  3. try boom(2) is run and throws an error
  4. try boom(1) is cancelled and its error discarded
  5. The error from try boom(2) is rethrown.

In practice I wouldn’t complain if boom(1) wins. I also don’t have any expectations either way here:

try await withThrowingDiscardingTaskGroup { group in
    group.addTask {
        await …
        try boom(1)
    }
    await …
    try boom(2)
}

unless I can reason about the await statements themselves in a way that makes this no longer a race (e.g. one is sleep(1000) and the other is sleep(2000)).

1 Like

This assumption is invalid, and it's always invalid in concurrent code.

It's entirely possible for the CPU scheduler to decide to put the thread running the outermost block to sleep immediately after addTask returns, and then run the child task to the point that it errors. That can allow it to throw first, even though there's no await.

5 Likes

Can you guys update the proposal that ThrowingDiscardingTaskGroup actually has a generic Failure: Error type. While parsing the proposal I had the impression that error type was completely discarded as well. I had to look up the actual implementation to find out that it's still there.