cancel should be done with outbound user event#110
Conversation
| 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!
weissi
left a comment
weissi
left a comment
There was a problem hiding this comment.
sorry, missed the more important bit first :)
| } | ||
| return nil | ||
| } | ||
| 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
2 participants
fixes #97