Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Cancelling ModelSendAsync can close the connection when it shouldn't #1750

Open
lukebakken opened this issue Dec 18, 2024 · 2 comments · May be fixed by #1753
Open

Cancelling ModelSendAsync can close the connection when it shouldn't #1750

lukebakken opened this issue Dec 18, 2024 · 2 comments · May be fixed by #1753
Assignees
@lukebakken
Labels
bug
Milestone
7.1.0

Comments

@lukebakken
Copy link
Contributor

lukebakken commented Dec 18, 2024 •
edited
Loading

Describe the bug

Reported here: #1720 (comment)

Reproduction steps

  • Pass cancellation token to BasicConsumeAsync that will close in the future.
  • Notice that the connection is closed: 
    CloseReason: AMQP close-reason, initiated by Library, code=541, text='A task was canceled.', classId=0, methodId=0, exception=System.Threading.Tasks.TaskCanceledException: A task was canceled.
at RabbitMQ.Client.Impl.BasicConsumeAsyncRpcContinuation.HandleCommandAsync(IncomingCommand cmd)
at RabbitMQ.Client.Impl.Channel.HandleCommandAsync(IncomingCommand cmd, CancellationToken cancellationToken)
at RabbitMQ.Client.Framing.Connection.ProcessFrameAsync(InboundFrame frame, CancellationToken cancellationToken)
at RabbitMQ.Client.Framing.Connection.ReceiveLoopAsync(CancellationToken mainLoopCancellationToken)
at RabbitMQ.Client.Framing.Connection.MainLoop()

Expected behavior

Connection should not close in this case.

Additional context

#1720 (comment)
@lukebakken lukebakken added the bug label Dec 18, 2024
@lukebakken lukebakken self-assigned this Dec 18, 2024
@lukebakken lukebakken added this to the 7.1.0 milestone Dec 18, 2024
@ZajacPiotr98
Copy link

I think passing already cancelled cancellation token will trigger exception already on the await _rpcSemaphore.WaitAsync(k.CancellationToken). The problem occurs when it's passed to the Connection.

Any exception thrown from the HandleCommandAsync method of AsyncRpcContinuation is causing closing the Connection.

lukebakken added a commit that referenced this issue Dec 26, 2024
@lukebakken
Fix cancellation of RPC methods
f331ddf 
Fixes #1750

* Start by adding a test that reproduces the error. Give a 5ms cancellation to `BasicConsumeAsync`, with a much longer delay via Toxiproxy. If running in debug mode, you will see the same `task canceled` exception, but it does not propagate to the test itself.
lukebakken added a commit to rabbitmq/rabbitmq-server that referenced this issue Dec 27, 2024
@lukebakken
Add hack to test rabbitmq-dotnet-client-1750
1f13661 
See rabbitmq/rabbitmq-dotnet-client#1750
@lukebakken lukebakken linked a pull request Dec 27, 2024 that will close this issue
Open
lukebakken added a commit that referenced this issue Dec 27, 2024
@lukebakken
Fix cancellation of RPC methods
a3618c6 
Fixes #1750

* Start by adding a test that demonstrates the error. Give a 5ms cancellation to `BasicConsumeAsync`, with a much longer delay via a hacked RabbitMQ. If running in debug mode, you will see the same `task canceled` exception, but it does not propagate to the test itself.
* Set cancellation correctly for TaskCompletionSource in AsyncRpcContinuation
@lukebakken lukebakken mentioned this issue Dec 29, 2024
Open
@lukebakken
Copy link
Contributor Author

@ZajacPiotr98 if you have time to test #1753, I would appreciate it. Thanks!

lukebakken added a commit to rabbitmq/rabbitmq-server that referenced this issue Jan 3, 2025
@lukebakken
Add hack to test rabbitmq-dotnet-client-1750
40a9cd0 
See rabbitmq/rabbitmq-dotnet-client#1750
lukebakken added a commit that referenced this issue Jan 3, 2025
@lukebakken
* Add wait on RegisteredAsync to see how the changes in #1750 affec…
b021daa 
…t the order of operations.
lukebakken added a commit that referenced this issue Jan 3, 2025
@lukebakken
Fix cancellation of RPC methods
93f28ed 
Fixes #1750

* Start by adding a test that demonstrates the error. Give a 5ms cancellation to `BasicConsumeAsync`, with a much longer delay via a hacked RabbitMQ. If running in debug mode, you will see the same `task canceled` exception, but it does not propagate to the test itself.
* Set cancellation correctly for TaskCompletionSource in AsyncRpcContinuation
lukebakken added a commit that referenced this issue Jan 3, 2025
@lukebakken
* Add wait on RegisteredAsync to see how the changes in #1750 affec…
1d4d179 
…t the order of operations.
lukebakken added a commit that referenced this issue Jan 10, 2025
@lukebakken
Fix cancellation of RPC methods
c684aae 
Fixes #1750

* Start by adding a test that demonstrates the error. Give a 5ms cancellation to `BasicConsumeAsync`, with a much longer delay via a hacked RabbitMQ. If running in debug mode, you will see the same `task canceled` exception, but it does not propagate to the test itself.
* Set cancellation correctly for TaskCompletionSource in AsyncRpcContinuation
lukebakken added a commit that referenced this issue Jan 10, 2025
@lukebakken
* Add wait on RegisteredAsync to see how the changes in #1750 affec…
831ab79 
…t the order of operations.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@lukebakken lukebakken

Labels
bug
Projects
None yet
Milestone
7.1.0
Development

Successfully merging a pull request may close this issue.

Handle OperationCanceledException in RPC continuations rabbitmq/rabbitmq-dotnet-client
2 participants
@lukebakken @ZajacPiotr98