Add Channel.ConsumeWithContext to be able to cancel delivering #192
Conversation
|
Thanks! We'll take a look as soon as we have time. |
|
@lukebakken perhaps we could introduce this in a minor version, since it deprecates |
|
@t2y just dropping a message to let you know that I have not forgotten about this PR. During my QA of this PR, I found something odd regarding message re-queueing, and I'm investigating that before merging. Plus past weeks I was pulled onto something else. |
May have found a problematic behaviour with this implementation
|
@Zerpet Thank you for testing! I'm going to fix it when the problem can be reproduced. |
There was a problem hiding this comment.
Never mind, the behaviour is correct. It is important to note that cancelling a consumer will not re-queue in-flight messages. The app using this library must close the amqp channel to re-queue in-flight messages.
I would like to have a check on context cancellation before calling ch.call(...), as discussed in #124, before merging this PR.
There was a problem hiding this comment.
I realised that this PR changes the semantics of consume, into something that is not correct.
Previously, Consume would create a subcription in the server; such subscription will be valid until the channel is closed, or Cancel() was called. After this PR, the subcription will be valid until the channel is closed, or Cancel() is called, or the context gets cancelled.
We can't accept this PR with the new semantics, and it doesn't seem correct to use context cancellation to cancel a consumer. The context passed to ConsumeWithContext() should be used to cancel the call to ConsumeWithContext(), but it should not cancel a consumer after ConsumeWithContext() has returned successfully.
In other words, the go routine initiated in ConsumeWithContext() will have an impact on the consumer after ConsumeWithContext() returns successfully, and that is not correct. A consumer should be able to keep on receiving messages after the context in ConsumeWithContext is cancelled.
From the conversation in #124, the best way forward is to check for context cancellation before calling ch.call(...).
In addition, and optionally, we could wrap ch.call(...) in a go routine and observe context cancellation, or a signal from the wrapped call. The clean up tasks would be to close the AMQP channel. I'm not sure about this bit, please hold on from implementing this bit.
|
Thank you for describing the background. I will follow you if this PR is not valid for consumption. Before that, I want to confirm a little more as a reference. I understood existing At first, I thought |
|
I agree, by canceling Context I hope that everything below will also be canceled |
I would expect context cancellation to "cancel everything" ... i.e. do a |
I believe this is a coincidence. The original author of this package
The "catch" here IMO is that That being said, if folks find useful to cancel a consumer after the context expires, even when |
I agree with you. I updated the description of functions (I'd like to ask anyone to rewrite the proper content since I'm not a good English writer). |
There was a problem hiding this comment.
We haven't seen much activity on this PR for a while. Based on the previous feedback, I'm going to merge as-is and make some adjustments to docs and to original Consume function. We should be able to release this in a minor version of 1.x.
Thanks everyone for getting involved!
After #192, Consume was calling the new ConsumeWithContext function with a context.Background context. Whilst this would work, it would also pile up routines that would only return when the AMQP channel is closed. This is unnecessary, and some code duplication is preferrable over piling up blocked routines. Some clarifications to function documentation, and added new paragraphs to ConsumeWithContext function to explain the semantics. Signed-off-by: Aitor Pérez Cedres <[email protected]>
This is a reference implementation for #124 (comment).