Expose a method to enable out-of-order Publisher Confirms

[benmoss]

Adds a new method, PublishAndAwaitConfirm, to channels which allows users to publish a message and wait on its confirmation.

There are two problems with the existing mechanisms, NotifyPublish and NotifyConfirm. The first is that they both only expose confirmations to the user in-order, making it impossible to wait for a single confirmation without also waiting for every previous confirmation. The second is that the way they relay the confirmation back to the user is difficult to correlate to the published message. They expose the delivery tag through the Confirmation struct, but the delivery tag of the publish is not exposed, forcing the user to implement their own delivery tag accounting in order to correlate a publish and a confirm.

This implementation essentially returns a future for the confirmation from the PublishAndAwaitConfirm call, making it easy for users to correlate the publish to the confirm and hiding all of the delivery tag accounting to inside the library.

cc @ChunyiLyu @ikvmw

Sample usage:

ch.Confirm(false)
confirm, _ := ch.ConfirmPublish(...)
if !confirm.Wait() {
  log.Fatal("publish was nacked!")
}

One remaining question I have is if we want to deprecate NotifyPublish/NotifyConfirm. I believe that it'd be pretty straightforward to build in-order confirmations on top of this, so it's a superset of those APIs.

[michaelklishin]

If ConfirmPublish is a Publish variation I would like to see a clearer name. The name right now makes it sound like the function confirms something (a publish) to RabbitMQ but that's not at all how publisher confirms work.

[michaelklishin]

The name ConfirmPublish is confusing: it makes it sound like the user confirms something.

PublishAndAwaitConfirm or something similar would be clearer.

[lukebakken]

Could we get complete example code that demonstrates how this improves upon the current confirmation mechanism? Or, how the current mechanism is not sufficient for a particular use-case?

[deadtrickster]

@lukebakken more context: https://vmware.slack.com/archives/C055BSG8E/p1639479523081000 knative-extensions/eventing-rabbitmq#334 and knative-extensions/eventing-rabbitmq#543

[benmoss]

@lukebakken I edited the initial post with more description of the problem. I don't know if example code demonstrating this would be as clear as the description.

[benmoss]

A potential optimization I decided against implementing was to keep track of which confirmations are outstanding, which would allow us to do a O(1) lookup on this map rather than looping through every key. My hunch is that this would not be that much of a bottleneck, but it's just a hunch.

[deadtrickster]

we scanning here over whole d.confirmations because range on maps is unordered, can it be a potentiall hot spot?

[benmoss]

Yeah, if we were to have a huge amount of pending confirmations and received lots of small multi-confirms this could be inefficient, since we would be cycling through the whole map for each multi-confirm but skipping most of the keys.

My hunch though is that:

1. If there's a large amount of pending confirmations Rabbit will probably send a small number of large-range multi-confirms
2. We generally won't have a large amount of pending confirmations

[samyonr]

@benmoss note that the delivery tag is exposed since the last release.

With GetNextPublishSeqNo the user doesn't need to implement a delivery tag accounting, and can work with NotifyPublish and NotifyConfirm to create a map of pending delivery tags and their messages (and if needed, re-publish unconfirmed messages).

Regardless, having PublishSync (that blocks until confirmed) or even allowing to decide whether to wait or not as you did in PublishAndAwaitConfirm can be a nice add-on. Deprecating NotifyPublish/NotifyConfirm sounds a bit drastic since they are valuable when combined with GetNextPublishSeqNo.

[benmoss]

@samyonr the problem with GetNextPublishSeqNo is that it's still not possible to implement a thread-safe way of correlating the delivery tag of the Publish call. Two threads could both call GetNextPublishSeqNo on a channel and get the same tag, but only one of them will then publish with that tag.

It might be true that a user shouldn't share channels across threads, but this library does use mutexes to make it possible to do so. It's frustrating to me that the RMQ docs merely suggest that "for applications that use multiple threads/processes for processing, it is very common to open a new channel per thread/process and not share channels between them", it doesn't explain at all why this would be a common practice.

[lukebakken]

Regardless, having PublishSync (that blocks until confirmed)

This exists in the .NET client and results in users complaining about performance. We then have to explain that handling confirms async is the best method, etc etc. Please don't add that function to this library.

[lukebakken]

PublishAndAwaitConfirm implies that the function waits for the confirmation. What about PublishReturningDeferredConfirmation? Yep it's long but don't we all use auto-completion these days?

[benmoss]

Yeah, this is not PublishSync, this returns a future, so is still an asynchronous API. Yeah, I had similar reservations about PublishAndAwaitConfirm, but couldn't think of anything better.

[deadtrickster]

maybe ConfirmablePublish?

[benmoss]

PublishWithDeferredConfirm is my latest attempt, wdyt

[samyonr]

@benmoss yeah, got it - I imagined using locks to get the next delivery tag, but it's indeed a pitty since the library is already thread-safe (BTW, I saw some "best-practices" articles that argue to "Make sure that you don’t share channels between threads as most clients don’t make channels thread-safe (it would have a serious negative effect on the performance impact)", which isn't relevant here.

Regarding PublishSync, it's surprising that it's not clear that it affects performance. I thought that it's clear. Since it appears that it isn't, better to avoid.

PublishWithDeferredConfirm sounds fine IMO.

[lukebakken]

@deadtrickster @michaelklishin please note I'm trying to make the most of GitHub milestones -

https://github.com/rabbitmq/amqp091-go/milestone/2

Suggested here:

#29