Track publisher confirmations automatically

[lukebakken]

Fixes #1682

• Remove ConfirmSelectAsync from IChannel
• Add parameters to enable confirmations on IConnection.CreateChannelAsync

[Tornhoof]

IConnection.CreateChannelAsync

I wonder if an enum wouldn't be better, something like PublisherConfirmation.None, Automatic, Manual.
Mostly because two bools can model 4 states and I think we only need three and two bools closely related with similar names always mean, I mix them up and use them wrongly. ;)

[lukebakken]

The current API is ONLY to facilitate removal of ConfirmSelectAsync, that's it! I'm sure @danielmarbach or @bording will have something different.

@danielmarbach / @bording - how should we coordinate work on this feature?

[danielmarbach]

@lukebakken what was your plan with this PR? Was your idea to get this going and then eventually get reviews etc from our end?

Regarding the options, I think it might be best to have something like @Tornhoof mentioned to better represent the states of options that are possible if the current configurability needs to be preserved.

Since it is nowhere explicitly stated, I wanted to ask if you and your team have discussed why confirms and tracking should be individual options or even allowed to be turned off? Assuming confirms is the default, why would someone then not want to have tracking enabled? Is it purely to preserve options that allow to opt in to not so safe options to get more speed?

[lukebakken]

Is it purely to preserve options that allow to opt in to not so safe options to get more speed?

Yes, that's it. I'll get started on this today.

[bording]

I can see allowing publisher confirms to be enabled or not, but I would argue that once you've enabled them, the client should just handle tracking them for you now that the async API allows for that to be done in a per-message way.

[danielmarbach]

@lukebakken how do you intend to expose the number of outstanding confirms? Or is this something you want to follow up in another PR?

[lukebakken]

🤷 I'm just now switching gears from working on our AMQP 1.0 Go client 😵‍💫

[danielmarbach]

Looking at all the things required to pass at channel creation I think it might be better to have a channel creation options type/class similar to Azure SDK options types to configure various client types. Adding more overloads with primitive types seems to burry things and make the usage unnecessary complex

[danielmarbach]

@lukebakken I pushed the options type as a small contribution since you thumbed that up

Regarding

There will be a maximum count of outstanding confirmations allowed (also configurable). When this maximum is reached, calls to BasicPublishAsync will immediately return an exception.

What is the relationship of PublisherConfirmationsEnabled and PublisherConfirmationTrackingEnabled to the number of outstanding confirms? Is the idea that there will be some sort of MaximumNumberOfOutstandingPublisherConfirms that is set to an initial value and get be set to null which then means infinite?

You wrote something about the publish starting throwing an exception. Why not simply let them wait until new slots are available? A cancellation token passed to the publish would allow enforcing some sort of timeout anyway? Or does it have to be more sophisticated?

[danielmarbach]

I wanted to use init only members on the option type but realized that would require something like Polyfill or Polysharp to fill the gaps.

[lukebakken]

@danielmarbach - thanks for taking a look and contributing. I have been sidelined by RabbitMQ support this week.

What is the relationship of PublisherConfirmationsEnabled and PublisherConfirmationTrackingEnabled to the number of outstanding confirms? Is the idea that there will be some sort of MaximumNumberOfOutstandingPublisherConfirms that is set to an initial value and get be set to null which then means infinite?

I haven't thought this through all the way but that sounds right.

• A way to totally disable publisher confirmations
• A way to enable confirmations, but disable internal tracking
• When internal tracking is enabled, allow setting a limit to the outstanding number of confirms

You wrote something about the publish starting throwing an exception. Why not simply let them wait until new slots are available? A cancellation token passed to the publish would allow enforcing some sort of timeout anyway? Or does it have to be more sophisticated?

A wait with a timeout sounds great to me. If a token isn't passed to BasicPublishAsync we should have a default timeout value, like 5 seconds.

[danielmarbach]

we should have a default timeout value, like 5 seconds.

Why is that? In the world of cooperative cancellation, the notion of a timeout is usually expressed by passing a token that is attached to a token source that enforces the timeout. Then everything is in the hand of the caller. If you introduce another arbitrary way of setting a timeout you need to deal with different overloads of the method.

Or is the intent to build a similar API to SemaphoreSlim.WaitAsync that allows having a timeout overload which makes the method return after the timeout without throwing (then you would have to have some kind of return value indicating the publish was not successful within the defined timeout which also sounds a bit weird but maybe I'm not understanding things clearly)

[lukebakken]

Why is that? In the world of cooperative cancellation, the notion of a timeout is usually expressed by passing a token that is attached to a token source that enforces the timeout.

Ah, OK. We have places in the API where a TimeSpan can be passed for a timeout, but maybe we should re-re-review the public API for that and just expect users to know to use CancellationTokens.

[paulomorgado]

I wanted to use init only members on the option type but realized that would require something like Polyfill or Polysharp to fill the gaps.

We shouldn't be depending on any polyfill, specially when there are more than one known to exist, because it can create conflicts for users..

The compiler depends only on fully qualified names (that's why those libraries can exist) and the types don't even need to be public. The compiler itself generates some internal types rather than relying on a runtime specific types.

We can just grab the source of whatever types we need and add them as internal to the client.

That being said, init members are great for when you are initializing them, but there's no guarantee that they are initialized when consuming (you can still create instances with reflection). But like them for controlled use cases and I'd go for it.

But it's not that hard to use create these kind of types with mandatory constructor parameters.

[danielmarbach]

We shouldn't be depending on any polyfill, specially when there are more than one known to exist, because it can create conflicts for users..

The compiler depends only on fully qualified names (that's why those libraries can exist) and the types don't even need to be public. The compiler itself generates some internal types rather than relying on a runtime specific types

We can just grab the source of whatever types we need and add them as internal to the client

Those statements seem to contradict each other. The polyfil libraries I linked are source only dependencies and by default are internal. There is no clash with public types and effectively the copying is no longer necessary because that has already been done by the authors of the polyfil libraries in a considerate and consumable way.

I'm not saying copying is not an option. In fact I mentioned that on the issue I opened. I'm commenting directly on the points you brought up as a counter argument to not take a Dependency. There are other downsides of taking a Dependency to those source packages that are worth discussing though.

FYI following through your reasoning it would also be necessary to remove the private assets only NuGet Dependency to Nullable which the client takes a Dependency on today

[danielmarbach]

Why is that? In the world of cooperative cancellation, the notion of a timeout is usually expressed by passing a token that is attached to a token source that enforces the timeout.

Ah, OK. We have places in the API where a TimeSpan can be passed for a timeout, but maybe we should re-re-review the public API for that and just expect users to know to use CancellationTokens.

I think there are valid reasons for timeouts when the underlying mechanism uses / enforces timeouts like HTTP, TCP etc. In the example with publisher confirms it feels forced but again I might be misunderstanding things

[paulomorgado]

We shouldn't be depending on any polyfill, specially when there are more than one known to exist, because it can create conflicts for users..

The compiler depends only on fully qualified names (that's why those libraries can exist) and the types don't even need to be public. The compiler itself generates some internal types rather than relying on a runtime specific types

We can just grab the source of whatever types we need and add them as internal to the client

Those statements seem to contradict each other. The polyfil libraries I linked are source only dependencies and by default are internal. There is no clash with public types and effectively the copying is no longer necessary because that has already been done by the authors of the polyfil libraries in a considerate and consumable way.

I'm not saying copying is not an option. In fact I mentioned that on the issue I opened. I'm commenting directly on the points you brought up as a counter argument to not take a Dependency. There are other downsides of taking a Dependency to those source packages that are worth discussing though.

FYI following through your reasoning it would also be necessary to remove the private assets only NuGet Dependency to Nullable which the client takes a Dependency on today

Sorry, I assumed they were libs, without checking.

Polyfill looks like an all or nothing with the only option being making the code public or not.

I haven't tested Polysharp, but if it works as advertised, looks good.

[lukebakken]

@danielmarbach @paulomorgado we won't be adding any polyfill libraries to this project unless they are absolutely necessary for a feature. I hope to continue with this PR soon. Thanks!

[paulomorgado]

@danielmarbach @paulomorgado we won't be adding any polyfill libraries to this project unless they are absolutely necessary for a feature. I hope to continue with this PR soon. Thanks!

@lukebakken,

Those are not libraries (that was my mistake). They are tools.

Polyfill adds everything as source:

The downside there, as I see it, is that it requires C" 12.0.

Polysharp generates the polyfills:

And can be used with C# 10.0.

But, in the end, all we need is:

namespace System.Runtime.CompilerServices
{
    [global::System.ComponentModel.EditorBrowsable(global::System.ComponentModel.EditorBrowsableState.Never)]
    [global::System.Diagnostics.CodeAnalysis.ExcludeFromCodeCoverage]
    internal static class IsExternalInit
    {
    }
}

With a few lines of MSBuild it's possible to remove the extra files from either tool

[lukebakken]

"init only members" seems like a "nice to have" code feature, and not a "must have".

[lukebakken]

@danielmarbach @bording I've just committed support for basic.return and changed the BasicPublishAsync return value to be ValueTask<bool> - true if the message was routed and ack-ed, false otherwise.

I'd appreciate a review, thanks!

This API is not ideal for a library user, because it's up to them to correlate publishes with the associated ValueTask<bool> instances. However, it's not like we have a "message ID" other than publish sequence number to do that ourselves.

Thoughts? Leave that up to users?

[danielmarbach]

@lukebakken I see what I can do tomorrow before going on vacation for a week. Today it's already a bit late for me to review things

[bpolitaiev]

ase versions. Unless an extremely severe performance penalty can be demonstrated with the current API, it won't chang

Hey @lukebakken, thanks for the quick response. My point is that Nack is not unexpected scenario, as overflow | reject-publish is the only way to wait when queue is full.

[lukebakken]

@bpolitaiev I am going to take our conversation to a dedicated discussion. In general, following-up on a closed issue or PR isn't a good idea on GitHub.

[lukebakken]

@bpolitaiev @bording @danielmarbach -

#1713