[Channel] improve send cancellation #184
Conversation
|
|
||
| enum Emission { | ||
| case idle | ||
| case pending([UnsafeContinuation<UnsafeContinuation<Element?, Never>?, Never>]) | ||
| case pending(Set<Pending>) |
There was a problem hiding this comment.
hmm making this a set becomes non-deterministic on the order of application - which will make it hard to test. Do we need set uniqueness to prevent issues or would just an Array work?
There was a problem hiding this comment.
Unfortunately we have to be able to uniquely identify the pending operations so we can remove/resume them in case of cancellation thanks to their "id".
Using an array as a FIFO could be nice to guarantee deterministic operations when dequeuing but we cannot rely on the index of a newly added operation to potentially remove it in case of a cancellation because the array could be updated by another concurrent send/next operation in the meantime.
We would have to rely on the id in the Pending/Awaiting structure, and in case of a cancellation we would have to remove the element that matches the id. We loose the O(1) complexity of a Set for an O(n) :-(
I guess we could use something like OrderedSet but it would imply a dependency on swift-collections which we don't want I presume. Do you think we should implement a custom storage that fits our needs here ?
There was a problem hiding this comment.
@phausler did you have the change to think a bit about having an external dep ?
There was a problem hiding this comment.
OrderedSet is perfectly fine in my book.
There was a problem hiding this comment.
Nice, then I’ll update this PR to integrate it.
Thanks for your reply.
There was a problem hiding this comment.
@phausler I have successfully introduced OrderedSet in this PR
There was a problem hiding this comment.
@phausler do you think we have a go for this one, now that we use OrderedSet ?
There was a problem hiding this comment.
yea I think we are good to go here
|
|
||
| enum Emission { | ||
| case idle | ||
| case pending([UnsafeContinuation<UnsafeContinuation<Element?, Never>?, Never>]) | ||
| case pending(Set<Pending>) | ||
| case awaiting(Set<Awaiting>) |
There was a problem hiding this comment.
also the set of awaiting might pose issue too come to think of it... (I should have caught this earlier)
twittemb
force-pushed
the
improve/channel_send_cancellation
branch
from
9495d3f to
bde105c
Compare
twittemb
force-pushed
the
improve/channel_send_cancellation
branch
from
bde105c to
be52a2d
Compare
This PR aims to:
next()andsend()operations. Previously, asend()cancellation was terminating all the pending and awaiting operations, which seemed wrong to me as mentioned here [AsyncChannel] Question: is task cancellation in sending operations too violent? #182.nextwas added to the awaiting list, so it can be immediately removed by the operation handling. Although it seemed to work well, it was not obvious when reading the code, and I tried to make it more explicit, so the maintenance is easier.