cancel should be done with outbound user event #110
Conversation
| @@ -476,7 +476,7 @@ extension HTTPClient { | |||
| self.lock.withLock { | |||
| if !cancelled { | |||
| cancelled = true | |||
| channel?.pipeline.fireUserInboundEventTriggered(TaskCancelEvent()) | |||
| channel?.triggerUserOutboundEvent(TaskCancelEvent(), promise: nil) | |||
There was a problem hiding this comment.
I think since cancellation is not guaranteed to do anything (it's best effort only), we can skip any errors, wdyt?
There was a problem hiding this comment.
@artemredkin firing through the ChannelPipeline should come out of the lock. So something like
let channel = self.lock.withLock {
if !self.cancelled {
self.cancelled = true
return self.channel
}
}
channel?.triggerUserOutboundEvent(...)
Regarding ignoring the promise: I'm okay with that.
There was a problem hiding this comment.
fixed, thanks!
| } | ||
| channel?.triggerUserOutboundEvent(TaskCancelEvent(), promise: nil) |
There was a problem hiding this comment.
@artemredkin I'm pretty sure this doesn't actually work (which proves there's no test) because whatever ChannelHandler is supposed to catch this event needs to change from inbound to outbound user event.
There was a problem hiding this comment.
we need to add a test that verifies cancellation works.
There was a problem hiding this comment.
we definitely have the test, it just never finishes :D
There was a problem hiding this comment.
fixed! and tests pass now
fixes #97