Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use context for Publish methods #96

Merged
merged 10 commits into from
Jul 13, 2022
Merged

Conversation

sapk
Copy link
Contributor

@sapk sapk commented Jul 7, 2022

Implement #92

This currently require context to be always cancelled (like net/http query) to not have stuck goroutine.

I am considering switching from waitgroup to the context of query for the sync pattern of DeferredConfirmation. This could lead to no leaking goroutine.

The latest implementation replace waitgroup of deferred with context so that we don't need goroutine to monitor context.

@sapk sapk changed the title Draft: Use context for Publish methods Use context for Publish methods Jul 7, 2022
@michaelklishin
Copy link
Member

Thank you, this is a non-trivial contribution!

@sapk
Copy link
Contributor Author

sapk commented Jul 7, 2022

It introduce some breaking change on methods arguments by adding context and enforce an non-nil context on Publish methods.

I enforce it because someone should never use a nil context and I feel it should failed early on that.
From context documentation:

Do not pass a nil Context, even if a function permits it. Pass context.TODO if you are unsure about which Context to use.

Even so, I understand that it could be better to support it if you feel it is needed. We could change it to support nil context by creating a background context in DeferredConfirmation in case of nil or an other solution is only require it when needed in case of ch.confirming == true.

@michaelklishin
Copy link
Member

We can bump minor or even major version to accommodate this change if it is accepted.

@guidog
Copy link

guidog commented Jul 11, 2022

Changing the API out of the blue is not nice.

I suggest to rename the context based Publish and leave the old one in place.

Not every one wants to change their application (time, money, you name it).
Given how it looks like I'm quite confident that there won't be "backports" to older versions.

A little sense of stability would be nice.

@Zerpet
Copy link
Contributor

Zerpet commented Jul 11, 2022

Changing the API out of the blue is not nice.

I agree that those changes are not nice. However, a major version, according to semver, reflect a breaking change in the API. I am in favour of introducing this change in a major.

A little sense of stability would be nice.

The API has been stable almost since its inception in streadway/amqp. We have to find a balance between having stability and evolving the API. The addition of context.Context is a necessary step to unlock the potential of Open Telemetry and context propagation.

@Zerpet Zerpet self-requested a review July 11, 2022 11:25
@lukebakken lukebakken added this to the 2.0.0 milestone Jul 11, 2022
@lukebakken lukebakken self-assigned this Jul 11, 2022
@sapk
Copy link
Contributor Author

sapk commented Jul 11, 2022

I could do a Publish and PublishWithContext and manage nil context case to be mergeable in this current major version if it is easier.
I could also indicate that publish methods without context will be deprecated to help prepare migration with later version.

Copy link
Contributor

@lukebakken lukebakken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sapk thank you for your work on this. A suggestion for you, @Gsantomaggio, @Zerpet and others (cc @fho) to consider, that may direct users of this library to use it correctly:

  • Do not change the API for Publish. If the user calls Publish with confirmations enabled on a channel, an error will be returned to indicate that PublishWithDeferredConfirm must be used.
  • If the user calls PublishWithDeferredConfirm when confirmations are not enabled, an error would be returned stating that confirmations must be enabled on the channel.

Or, add a new API keeping the existing one to preserve backwards-compatibility 🤷‍♂️

Thoughts?

@sapk
Copy link
Contributor Author

sapk commented Jul 11, 2022

I updated the PR to be backward compatible and rollback client test changes to validate working with previous Publish methods without context.
But kept using with context, in examples tests.

With Deprecated: comment, the methods without context should trigger linters if used to ease migration.

Deprecated: notice in documentation:

image image

Copy link
Contributor

@fho fho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer to change the API instead of introducing WithContext methods + deprecating the old ones.
It's a minor change and not much effort to adapt code when upgrading to new major amqp091-go release

channel.go Show resolved Hide resolved
confirms.go Show resolved Hide resolved
@lukebakken
Copy link
Contributor

@fho we will do that for version 2.0

@lukebakken lukebakken modified the milestones: 2.0.0, 1.4.0 Jul 12, 2022
@lukebakken lukebakken self-requested a review July 13, 2022 13:24
@lukebakken lukebakken merged commit 42c5149 into rabbitmq:main Jul 13, 2022
@sapk sapk deleted the issue-92 branch July 13, 2022 13:29
tie added a commit to tie/amqp091-go that referenced this pull request Dec 16, 2022
This change removes embedded context.Context (which is generally an
antipattern) 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 rabbitmq#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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants