rabbitmq / rabbitmq-stream-dotnet-client

RabbitMQ client for the stream protocol
https://rabbitmq.github.io/rabbitmq-stream-dotnet-client/stable/htmlsingle/index.html
Other
122 stars 41 forks source link

Let users provide `CancellationToken` #300

Open aeb-dev opened 1 year ago

aeb-dev commented 1 year ago

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

I created 100k streams to test the behavior of both the server and client. After I am done, I wanted to delete them with the client DeleteStream api. I used the following code:

foreach (var index in Enumerable.Range(1, 100_000))
{
    await ss.DeleteStream($"stream-{index}");
}

I got the following error:

System.TimeoutException: The operation has timed out.
         at RabbitMQ.Stream.Client.ManualResetValueTaskSource`1.System.Threading.Tasks.Sources.IValueTaskSource<T>.GetResult(Int16 token) in /_/RabbitMQ.Stream.Client/Client.cs:line 764
         at RabbitMQ.Stream.Client.Client.Request[TIn,TOut](Func`2 request, Nullable`1 timeout) in /_/RabbitMQ.Stream.Client/Client.cs:line 407
         at RabbitMQ.Stream.Client.Client.Request[TIn,TOut](Func`2 request, Nullable`1 timeout) in /_/RabbitMQ.Stream.Client/Client.cs:line 409
         at RabbitMQ.Stream.Client.Client.DeleteStream(String stream) in /_/RabbitMQ.Stream.Client/Client.cs:line 708
         at RabbitMQ.Stream.Client.StreamSystem.DeleteStream(String stream) in /_/RabbitMQ.Stream.Client/StreamSystem.cs:line 332

Describe the solution you'd like

My test probably is not something people will do but I think it is still valuable to have an option to provide CancellationToken

Describe alternatives you've considered

No response

Additional context

No response

Gsantomaggio commented 1 year ago

The timeout comes from the server, which takes time to execute the operation. I don't see how the CancellationToken can improve the situation here.

Creating the destroying 100k streams requires time, and maybe you need more resources on the server side, and it is a bit odd as a use case.

I am unsure if we will implement the feature, but we are open to pull requests, so feel free to propose the enhancement.

aeb-dev commented 1 year ago

The timeout comes from the server, which takes time to execute the operation. I don't see how the CancellationToken can improve the situation here.

How did you conclude this? Because when I ran the command it timed out but the server continued to delete streams.

Creating the destroying 100k streams requires time, and maybe you need more resources on the server side, and it is a bit odd as a use case.

I agree that my case is niche however CancellationToken can be used for graceful shutdown, cancelling an operation if the operation is time sensitive etc.

I am unsure if we will implement the feature, but we are open to pull requests, so feel free to propose the enhancement.

I will try to check, if I can allocate time

Gsantomaggio commented 2 months ago

@lukebakken any thought on this?

lukebakken commented 2 months ago

Yes, all async methods should take CancellationToken as a parameter. They should also end with the Async suffix. I've added this to the 2.0 milestone since it will involve breaking API changes.