Do not embed context in DeferredConfirmation

[tie]

This change removes embedded context.Context (which is generally an anti-pattern) from DeferredConfirmation. Instead, we use a simple channel to wait for ack/nack status. This approach is more flexible since it can be combined with timers, tickers, other contexts and channels in general using select{} statements and there is no overhead from context cancellation setup.

Note that #96 introduced a behavior where Wait would unblock and return false once the context passed to Publish expires. This commit reverts this (arguably breaking) behavior in favor of WaitContext function.

[tie]

Cc @sapk as you are the author of #96 and may be interested in this change.

[Zerpet]

Thank you for contributing to this project. I like this idea of using channels instead of the embedded context. However, I'm not a fan of using sync/atomic, unless it is strictly necessary. Have you tried to use mutexes instead?

[tie]

On a second look, we don’t actually need other synchronization primitives as ack is already guarded by done channel. A non-blocking select with default clause in Acked method is enough.

[Zerpet]

This is great and very clever use of channels 👍

Since this is a breaking change (change to public function signature), we will merge this for version 2.0.0. That will probably be after we release 1.6.0, which should be after we merge #145

[tie]

I think we can make the change completely backwards compatible by

• using sync.Once in setAck and marking DeferredConfirmation.Confirm as deprecated (again, I don’t think it’s used or is useful outside of amqp091-go internals),
• initializing underlying channel to ctx.Done when called from Publish*WithContext functions, but marking them as deprecated (in favor of Publish followed by WaitContext or select on Done) and un-deprecating regular Publish variants until v2 release—I assume you are planing to include context.Context argument for all functions that perform I/O operations under the hood, without *WithContext cruft,
• when v2 is released, remove the behavior above (storing context in DeferredConfirmation).

What do you think?

It’d also be nice to add DeferredConfirmation.Cancel or Channel.CancelDeferredConfirmation to remove it from the pending deferredConfirmations map, but that is an extremely minor resource usage optimization I don’t think is worth implementing.

[tie]

Since this is a breaking change (change to public function signature)

I don‘t think that this PR introduces breaking changes aside from removing DeferredConfirmation.Confirm and context.Context cancellation propagation to DeferredConfirmation from Publish*WithContext. Both can be addressed as described in the comment above.

I didn’t run apidiff for this PR, so am I missing something?

[Zerpet]

After having a second look, you are right, this PR is arguably breaking. It is true that we are removing DeferredConfirmation.Confirm, but I think it shouldn't have been exported in the first place. The function signature change is in unexported structs, relevant only to the library internals. Your suggestions would work, but I think it's unnecessary. I'm happy to merge this PR as-is after 19e503a

[Zerpet]

@lukebakken tagging you since you assigned yourself first 🙂 Are you happy with the outcome of this conversation?

In summary:

• Removal of DeferredConfirmation.Confirm should not impact users. This function should be used only by this library.
• The main use case of Use context for Publish methods #96 and DeferredConfirmation is to DeferredConfirmation.Wait() on individual confirmations. This PR does not break this use case.
• This PR corrects the Go anti-pattern of embedding context.Context

[lukebakken]

@Zerpet I don't see where 19e503a has been merged, nor a PR for it 🤔

[Zerpet]

19e503a is part of this PR, it addresses my first round of feedback 😄

[lukebakken]

That's weird, when I originally clicked on the commit github said it did not exist on any branch. It's working now at least.

[sapk]

Sorry, I am late on the subject.

Yes this is not the idiomatic way to use context in struct but it was the only way I found to cancel the confirmation without too much breaking.

This PR is clearly a better implementation.

The only downside I see is that we can't really expire a confirmation and it would be kept open until next message with multiple ack but the wait can still be non blocking which still solve the main problem I encounter.

Sidenote: since context is not needed for publish anymore, PublishWithContext and PublishWithDeferredConfirmWithContext could be removed on the other side it could be kept for later to be able adding tracing for example.

[sapk]

Also probably examples need to be updated as they may imply some logic that is not really true anymore since context on publish is ignored.
Ex: https://github.com/rabbitmq/amqp091-go/blob/main/_examples/simple-producer/producer.go#L104-L111

[Zerpet]

The only downside I see is that we can't really expire a confirmation and it would be kept open until next message with multiple ack but the wait can still be non blocking which still solve the main problem I encounter.

I'm not sure I understand this point. Do you mean that, the removal of DeferredConfirm.Confirm(bool), removes the ability to "give up" on a specific confirmation?