Version 7 API - issue with mixing sync / async in same API

[lukebakken]

cc @michaelklishin @stebet @paulomorgado @bollhals @danielmarbach @bording @pabermod @plewam @Zerpet @Gsantomaggio @eerhardt @davidfowl

Hi everyone,

I am cc-ing everyone who has recently made a major contribution or has given input on issues or pull requests.

I have been making slow and steady progress on version 7 of this client. Currently, I'm making sure that Async methods take a cancellation token, and use it correctly (the latter part is the difficult part, of course).

Throughout the course of getting version 7 ready, I have run into issues where tests that exercise the sync-over-async API fail to run, usually on the net472 target. See this workflow run, which was cancelled at the 6 hour time limit:

https://github.com/rabbitmq/rabbitmq-dotnet-client/actions/runs/7508636025/job/20444501415

I have pretty much reached the end of my patience for having a sync and async API in the same set of interfaces and assemblies. Team RabbitMQ is small, and in addition to working on this library (and others) I do a lot of support for commercial RabbitMQ customers. I would like to get version 7 of this library shipped in a reasonable amount of time so I am going to propose that we do one of the following for version 7:

• Make version 7's API async only. If a synchronous API is needed, users can stay on version 6. We will establish an end-of-life date for version 6 when version 7 ships. This is my preferred solution.
• Split all of the code into an async API and non-async API. This can be done at the namespace level, assembly level, whatever.

Make your opinions known here!

[lukebakken]

cc @kjnilsson

[bording]

Make version 7's API async only. If a synchronous API is needed, users can stay on version 6. We will establish an end-of-life date for version 6 when version 7 ships. This is my preferred solution.

I agree that this makes sense. We did the same thing in NServiceBus years ago.

[lukebakken]

Thank you for your input! I should have checked to see what NServiceBus did 😸

[pabermod]

Hi,

I think the way forward when doing network operations is going full async.

Other libraries have gone full async like renci.ssh

If someone needs sync operations the calls can be wrapped, carefully to avoid deadlocks, by using for example a custom synchornization context like this one:

https://github.com/restsharp/RestSharp/blob/dev/src/RestSharp/AsyncHelpers.cs

[lukebakken]

Thank you for your input and contributions @pabermod

[danielmarbach]

Another approach mentioned before is to have a small core sync and async pipeline and the have async methods with flags that conditionally go on one or the other path to that core and the sync versions ensuring the task returned is always in completed state similar to the Azure SDK https://github.com/Azure/azure-sdk-for-net/blob/08ac84b398ca9d6f87a70c0d5c8ec8ea9acec191/sdk/storage/Azure.Storage.Queues/src/QueueClient.cs#L1436

[michaelklishin]

Async only makes sense to me (since this will be a new major).

[stebet]

Async only makes perfect sense to me.

[danielmarbach]

Sync only 🤣

Of course I'm trolling. The maintenance effort for a small team like yours to provide sync and async plus all the bugs that come with that decision due to the requirement of duplicating larger portion of of the code base is not worth it and would unnecessarily stall efforts in other areas for the sake of pleasing a very small and diminishing (shall we say diminished?) user base

[lukebakken]

I really appreciate the fast and unanimous feedback. I'll begin deleting code soon 😸

[lukebakken]

#1472

[paulomorgado]

Async all the way for the win!!!

Are you able to collect a memory dump of the hang process to determine where the hang is?

[eerhardt]

One scenario I wanted to call out is putting the IConnection instance in a DependencyInjection container as a singleton instance. Using Microsoft.Extensions.DependencyInjection, it isn't possible today resolve a service in an async manner. See dotnet/runtime#65656.

In .NET Aspire we are using IConnectionFactory.CreateConnection() to establish the connection and add the IConnection as a singleton. See https://github.com/dotnet/aspire/blob/e3fc7cc96166078b27ba9e63558761ef265a2fcd/src/Components/Aspire.RabbitMQ.Client/AspireRabbitMQExtensions.cs#L168 and an explanation about this in dotnet/aspire#565.

Would it be possible to keep this synchronous method to support putting RabbitMQ connections in DI?

[lukebakken]

I would be very interested in @bording's input here, especially given his comment here and experience with the NServiceBus project.

[danielmarbach]

FYI @bording and I share the same experience. Do you have any specific questions?

[lukebakken]

Oh, of course!

Did NServiceBus have to keep anything synchronous around to appease DI container libraries?

[paulomorgado]

@eerhardt, is there any problem in having sync-over-async in this situation?

Or injecting a Task<IConnection>?

[bording]

Did NServiceBus have to keep anything synchronous around to appease DI container libraries?

This mostly hasn't been a problem for us. We largely only use DI to create things that are provided by users of our libraries. Internally, we don't rely on DI for infrastructure like transport initialization.

[danielmarbach]

@eerhardt I would like to add I'm not against keeping a synchronous connection method. The intent of my comment here is to expand on what you wrote and provide another perspective. Hopefully my comment is well received.

As you have pointed out the .NET ecosystem had already with the introduction of the hosting and the dependency injection abstraction a general problem of lacking a good extension point to initialize connection means. Like you mentioned there is no way on the DI abstractions with the built in mechanics to asynchronously initialize a component when it is resolved or when you try to use the hosted services to do that you run into ordering problems due to the hosted services constructor dependencies being resolved for all the hosted services at once and then the hosted services being started sequentially or with the newest additions even concurrently. Many libraries out there that moved to an asynchronous only model suffer from the lack of this capability and there are various workarounds required to initialize components asynchronously.

So I wonder why it should be up to an individual library to keep certain APIs around, and having to deal with that complexity there as well as having to explain why there is a single sync IO-bound method still around instead of leaving it up the host entry point to deal with this problem? (I'm aware that there are a magnitude of more hosts around that then have to deal with it but again this seems to be a larger ecosystem problem anyway).

When I look at Aspire components it seems even there we can see this problem sometimes being addressed and sometimes not. For example when I look at the RabbitMQ component it establishes a connection as part of the component declaration while for example the SQL Client, MySQL component doesn't seem to try to open the connect before it is being resolved. This seems to indicate that are components that either rely on implicit initialization mechanisms of the component exposed, manually manage connection mechanisms as part of the Aspire wire-up or rely on the client of the exposed component to initialize the connections.

Given that Aspire is in control of the host and all the registrations couldn't Aspire too leverage abstractions like hosted services, lifecycle hooks of Aspire etc. to make sure the connection is established asynchronously too? Yes there is still the problem of ordering of the wire up vs the usage of the components order but that seems to be also generally a problem that the entire .NET ecosystem has today with the hosting and DI abstractions we have in place.

[paulomorgado]

@lukebakken

I am currently converting all of the tests to use the async API, then I will remove the sync API code.

If people have ideas about how to preserve a sync API with minimal maintenance necessary, by all means, open a PR after I'm done. Seems like some libraries have done it - #1471 (comment)

That is hiding the problem, not solving it: carefully to avoid deadlocks

If the sync and async methods are very similar, code generation is possible.

[lukebakken]

I look forward to PRs from the community for these ideas 😄

[lukebakken]

@paulomorgado really, I need to get the async API working perfectly before I can even entertain ideas about supporting a sync API. If it's truly that important, a community member can step up and contribute.

Also note that version 6 of this client has design flaws that requires increasing the ThreadPool size.

I'm sure everyone in this discussion is aware that an async-compatible version of this client has been in discussion for years. It's time to get it done.

[danielmarbach]

@eerhardt

The way the Azure ServiceBus client library handles this problem is that the the ServiceBusClient object can just be new'd up, and it remains unconnected until the first request, i.e. SendAsync on a ServiceBusSender. The ServiceBusCilent's Connection will then be opened, and remain open, until someone disposes the client.

IMO, this is the nicest API model for consumers. There is no explicit "open connection" operation. It happens automatically the first time it is needed.

I do agree. It is implicit managed connection associated with the parent factory. That model works well generally with the downside of having overhead on the first operations which can get exarbated on concurrent send scenarios quite a bit due to the required underlying synchronization. Plus it does not allow an informed caller to explicitly initialize the connection as part of a warmup without having to actually execute an operation. But generally I also find that model the cleanest from a user standpoint. It also simplifies DI wiring tremendously

[pabermod]

Also it separates the senders from the receivers. Given it is not a good a idea to share the same IModel for sending and receiving it would seem a good approach to separate them here