Add an algorithm to obtain the first result of a list of tasks #5
Conversation
phausler
requested review from
jmschonfeld,
kperryua,
natecook1000,
parkera and
timvermeulen
| } catch { | ||
| XCTAssertEqual((error as NSError).code, -1) | ||
| } | ||
| } |
There was a problem hiding this comment.
Should we test task cancellation as well?
There was a problem hiding this comment.
That is another aspect I am considering about this - should these first methods have a cancellation handler? if so that would make the return value optional.
or should that be a second set of functions?
There was a problem hiding this comment.
If a task is cancelled and "finishes first", then first will throw the CancellationError, no? Is that not enough? What did you have in mind?
There was a problem hiding this comment.
well I guess I am thinking of perhaps an initializer on Task that takes N blocks and returns an actual Task object that can be cancelled is one option. The other option is to pass a cancelHandler and make the return type optional.
There was a problem hiding this comment.
OK. We can discuss the shape of an API like that separately I think. In the meantime, with the current methods, is cancelling a task any different than the throwing case? Does it still deserve its own test case?
|
hmm I think I have a safer idea to explore that might address the cancellation - will be adjusting this PR accordingly |
…rts cancellation propigation
phausler
force-pushed
the
pr/task_first
branch
from
6ab2d28 to
b6f9d5c
Compare
| } else { | ||
| return task | ||
| } | ||
| } |
There was a problem hiding this comment.
Thoughts on putting the withCriticalSection call in this method proper? I think you could similarly add
mutating func set(continuation: ...) { }
mutating func removeAll() -> [Task<Success, Failure>]? { }
which similarly encapsulate the withCriticalSection invocation. I think this would do a lot to make the callsites more readable.
There was a problem hiding this comment.
The critical section needs to be outside of the mutation method because the mutation has the write back of the value of the structure so it needs to be external to the change.
| /// - Parameters: | ||
| /// - tasks: The running tasks to obtain a result from | ||
| /// - Returns: The first result from the running tasks | ||
| public static func first<Tasks: Sequence>( |
There was a problem hiding this comment.
This name, in conjunction with a sequence parameter, seems a little confusing, since one could easily read it as the result of the first task in the sequence. Have you thought about alternative names, e.g. firstResult(from: imageLoadingTasks)? It’s hard to disambiguate “first in order” from “first in time”!
There was a problem hiding this comment.
That is where the name race offers some better connotations; which it is a term of art in the FRP world.... sadly we (I think it is a fine name) don't per-se like those names.
There was a problem hiding this comment.
we can keep this internal for now since I was able to bring it into the same module.
|
As far as the name goes, |
4 participants
This is a fundamental building block for zip, combineLatest, debounce and numerous other operations on AsyncSequence, but is also useful generally for handling tasks.