rabbitmq / rabbitmq-dotnet-client

RabbitMQ .NET client for .NET Standard 2.0+ and .NET 4.6.2+
https://www.rabbitmq.com/dotnet.html
Other
2.1k stars 590 forks source link

[Feature Request] Please make sync APIs available again, do not force consumers and producers to block on async #1736

Closed iancooper closed 2 days ago

iancooper commented 2 days ago

Is your feature request related to a problem? Please describe.

async/await is not SDK "magic faerie dust". Whilst it is important to support it, it is also important to support sync APIs.

If you do not support synchronous APIs you impose a design decision on all consumers of your SDK: you must use asynchronous i/o or you have to block on an asynchronous operation. The problem here is that blocking on an asynchronous operation is far worse of a problem, than using blocking i/o.

An async/await programming model is viral; it forces the whole program, from top-to-bottom to become async. This may or may not be a valid choice, but as an SDK owner, you cannot determine this

In addition, in some cases, blocking i/o may be a better option, so you should leave this open for clients.

I note that the ASP.NET team had to roll back their decision to provide only non-blocking APIs, for exactly the same problem space.

Generally, SDK writers should not make choices for their users, as they cannot know their context. Specifically, they should not choose their concurrency model in this case. You can be opinionated, and prefer some options over others, but always provide an escape route to allow your opinions to be avoided.

You don't have the context to decide if a consumer should use async/await.

Describe the solution you'd like

A #dotnet SDK should support both blocking and non-blocking operations, so as to not force design decisions on consumers of that SDK, or force dangerous behaviors such as blocking on async.

Please put the synchronous APIs back into the SDK, with immediate effect.

Describe alternatives you've considered

One alternative here is to fork the SDK, and to put synchronous approaches back into the SDK. This seems to be an unhelpful response, so it would be better to discuss why this SDK change should be reconsidered.

Additional context

It is great that there are now asynchronous APIs. But the support for asynchronous APIs should not come at the expense of imposing design decisions on consumers of your SDK.

Tornhoof commented 2 days ago

I note that the ASP.NET team had to roll back their decision to provide only non-blocking APIs, for exactly the same problem space.

Just curious, which one do you mean, AllowSynchronousIO for HttpResponse et al?

Or do you mean the Sync Api for HttpClient in Runtime, which was heavily influenced by Azure SDK support back in the day. The HttpClient change only implements a limited set of sync functionality, for Http/1.1 there is a full sync path afaik, Http2/3 are only sync over async.

On Bluesky you wrote that for Message Consumers you have valid sync use cases, what are these? I'd understand Message Publishing for legacy code bases, as async is viral.

But why the consumers? They should be fairly easily adapted as you just need some kind of backgroundservice doing the async startup.

iancooper commented 2 days ago

I note that the ASP.NET team had to roll back their decision to provide only non-blocking APIs, for exactly the same problem space.

Just curious, which one do you mean, AllowSynchronousIO for HttpResponse et al?

You may be interested to follow this thread: The ASP.NET team was forced to begin rolling back their change from .NET 5, and more so in .NET 6.

Generally speaking, it is a bad idea to impose this decision on your users, because you cannot know their context. You do not know, as a library author, whether they should use async i/o or not. Some reasons that they might choose to be sync:

These are just some of the problems. The simple answer is that as a library designer, you cannot know.

iancooper commented 2 days ago

But why the consumers? They should be fairly easily adapted as you just need some kind of backgroundservice doing the async startup.

But you have now made an assumption about the range of possible ways that I can write my code.

async i/o is not "magic faerie dust". For example, local file i/o is generally faster via a blocking method than a non-blocking method, particularly as the CLR does not have truly asynchonous file i/o. A read-write to an SSD is faster via a blocking call than an async one.

Do you know all your user's contexts? Do you know all their paradigms under which they will use their code?

iancooper commented 2 days ago

As a framework author which uses RabbitMQ as one of our transports, and supports both sync and async for our users, because we don't want to assume their context, we would have to block over async to use this, or adopt a strategy of

var task = Task.Run(() => //rmq operation; task.Wait(); var response = task.Result;

around every RMQ call via an extension method wrapper for safety, to allow folks to continue to have a choice about their concurrency context.

Tornhoof commented 2 days ago

You may be interested to follow

Yeah that's the HttpClient change I was referring to, which was heavily influenced by Azure (as written in the first post and commented a lot in that thread) and specifically that the support is only partial.

But you have now made an assumption about the range of possible ways that I can write my code.

No I'm not making assumptions. I understand that you want to provide the users of your framework a sync api. As for SSDs, I think by now io_uring is largely comparable if not faster than epoll for NVMEs. See e.g. https://lore.kernel.org/io-uring/ZwW7_cRr_UpbEC-X@LQ3V64L9R2/T/ for a recent round of benchmarks

I'm just curious why you think message consumers being an interesting use case for sync, if I correctly understand your code in BrighterCommand's RMQ PullConsumer you already push all received messages into a ConcurrentQueue. If that use case is the interesting one, then you my suggestion above still applies.

But I also think that the async code in the current iteration of the RabbitMQ library is far superior than the previous code, both in readability/maintainability and performance metrics, as it uses a lot of the more modern functionality provided by the dotnet team in recent years.

Note: I don't have any skin in the game here, I just tried to provide you help to solve the problem of your sync consumer. Having said that, I leave the further discussion up to the maintainers.

iancooper commented 2 days ago

This seems to have been the problematic change: https://github.com/rabbitmq/rabbitmq-dotnet-client/issues/1472

iancooper commented 2 days ago

I'm just curious why you think message consumers being an interesting use case for sync

It may be worth reading David's comments here: https://bsky.app/profile/keansbox.com/post/3lc3lop3ewc2w

Async is a choice, and I make it when I want to choose co-operation over performance. In some cases, I may not want to choose co-operation. Software scales just fine without yielding to other threads all the time. For example, if I run a single-threaded worker in a container, I may well not choose to yield by using await; I may just rely on co-operative multi-tasking, because I want to optimise performance, reduce allocations etc.

The reality here is this: the RabbitMQ SDK CANNOT know my context. So dropping support for sync is a strong opinion about the types of RabbitMQ consumers you are prepared to support. The answer here from Broadcom is: if you have legacy needs, if you want performance over co-operation, if you want a simpler programming model: we don't care, please use something other than RabbitMQ for your workloads.