Particular / NServiceBus.Callbacks

https://docs.particular.net/nservicebus/messaging/callbacks
Other
2 stars 5 forks source link

When actual response type does not match the expected type, callback tasks hang indefinitely #586

Closed DorianGreen closed 2 months ago

DorianGreen commented 2 months ago

Symptoms

When a user awaits a callback task e.g.:

await bus.Request<MyResponse>(new MyRequest());

and the actual response received is different than the expected (i.e. MyResponse), the task will never be completed.

Who's affected

All users of:

Root cause

The problem is caused by the fact that the response-handling behaviors do not complete the completion sources for callback tasks when an exception is thrown while setting the result. Both when received response is a C# class:

https://github.com/Particular/NServiceBus.Callbacks/blob/6ca6db4080be89789a7d372c9a0af55e0d3c2eb8/src/NServiceBus.Callbacks/Reply/RequestResponseInvocationForMessagesBehavior.cs#L30

and when it is int or Enum

https://github.com/Particular/NServiceBus.Callbacks/blob/6ca6db4080be89789a7d372c9a0af55e0d3c2eb8/src/NServiceBus.Callbacks/Reply/RequestResponseInvocationForControlMessagesBehavior.cs#L37

Backported to

Fixed in

### Describe the bug #### Description When receiving a callback, and the response message type does not match the requested type, an ArgumentException is raised by the behaviors but the callback task is not aborted. Same happens if the reply is an int or enum and the user requested another callback type. #### Expected behavior https://github.com/Particular/NServiceBus.Callbacks/blob/6ca6db4080be89789a7d372c9a0af55e0d3c2eb8/src/NServiceBus.Callbacks/Reply/RequestResponseInvocationForMessagesBehavior.cs#L30 and https://github.com/Particular/NServiceBus.Callbacks/blob/6ca6db4080be89789a7d372c9a0af55e0d3c2eb8/src/NServiceBus.Callbacks/Reply/RequestResponseInvocationForControlMessagesBehavior.cs#L37 should be wrapped in a try catch, and ideally `result.Value.TaskCompletionSource.TrySetException` should be used to abort the awaiting task. the exception should include the control message value if possible #### Actual behavior An internal ArgumetException is raised in the receive pipeline and the corresponding callback request hangs #### Versions Fix required for 4.X and 5.X ### Steps to reproduce Sender endpoint calls `await messageSession.Request()` Receiver endpoint replies with int value 1 ### Relevant log output _No response_ ### Additional Information We require this behavior for upgrading legacy endpoints. When using callbacks, we try to handle the request. if the message cant be handled and is about to be sent to the error queue we reply with an int value to indicate a general error. by adding the proposed try catch to the behaviors, we can wrap the session.Request calls and handle the exception
tmasternak commented 2 months ago

@DorianGreen thank you for your contribution. We will look into the problem and get back to you with more information.

DorianGreen commented 2 months ago

Thanks @tmasternak for the quick response.

Would it be possible to wrap the exception in the control message path with a message that would include the control message value?

For message based replies it might be hard or impossible to include the body, but with control messages we have the actual value.

At least in out legacy case we would like to access the value and could extract it from the exception if possible

tmasternak commented 2 months ago

Hi @DorianGreen, regarding

(...) we would like to access the value and could extract it from the exception if possible

we discussed introducing this change and realized that this would likely have harmful consequences for the overall NServiceBus API usability.

The fact, that the expected message type and the received type do not match, likely indicates a bigger problem with the code, and in most cases we want our users to solve it at that (code change and redeployment) level.

Secondly, as you already mentioned this would be possible only for control-message-based responses, and would likely boil down to exposing the response as a string. This in turn would bring inconsistency to the callbacks API.

Finally, for control messages and normal messages, you can already write a custom pipeline behavior that could catch the casting error and have access to the incoming message - both headers and body.

tmasternak commented 2 months ago

Hi @DorianGreen,

we just released 5.0.1, 4.0.1, and 3.0.1 version on the package with the fix.