SE-0304 (3rd review): Structured Concurrency

Evolution Proposal Reviews
5

And I have a more serious concern:

func gather(first m: Int, of work: [Work]) async throws -> [WorkResult] { 
  assert(m <= work.count) 
  
  return withTaskGroup(of: WorkResult.self) { group in 
    for w in work { 
      group.async { await w.doIt() } // spawn child tasks to perform the work
    }  
    
    var results: [WorkResult] = []
    while results.count <= m { 
      switch try await group.nextResult() { 
      case nil:             return results
      case .success(let r): results.append(r)
      case .failure(let e): print("Ignore error: \(e)")
      }
    }
  }
}

In this code snippet from the proposal, the group.cancelAll() (in addition to the return results) has been mistakenly left out. During the history of this proposal, this exact mistake has been repeatedly made (at least three times) by the proposal authors as well as other members of this forum. Forgetting the group.cancelAll() would have no effect on the behavior of the code, only on its performance. Thus, it is a very hard to find bug.

I am a firm believer that good APIs must make it hard to make mistakes. Therefore, I think it should be an explicit choice for the programmer if the remaining tasks shall be cancelled or not.

(Obviously, in this case, the missing return statement would generate a compiler error. However, it is possible and maybe even likely that this would result in the programmer only adding the return statement to satisfy the compiler and still forgetting the group.cancelAll().)

One possible solution

Changing the return type of operation to an enum so that the return statement would either read return .cancellingRemainingTasks(result) or return .finishingRemainingTasks(result) would be ideal in my opinion, because it would virtually eliminate mistakes as seen above. Maybe there are less verbose options that achieve the same goal.

6 Likes