diameter: Call message_cb 'ack' with updated callback #9815
Conversation
CT Test Results 2 files 34 suites 12m 3s ⏱️ Results for commit da04f7c. ♻️ This comment has been updated with latest results. To speed up review, make sure that you have read Contributing to Erlang/OTP and that all checks pass. See the TESTING and DEVELOPMENT HowTo guides for details about how to run test locally. Artifacts
// Erlang/OTP Github Action Bot |
|
Hi, is it possible for you to add a test case that would detect this problem? |
|
@Mikaka27 yes, I can try |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a bug in Diameter transport modules where the message_cb callback is not properly updated for subsequent ack events when a new callback is returned during a send event.
- Reorders the execution of message sending and action processing to ensure updated callbacks are used
- Ensures that when
message_cbreturns a new callback during asendevent, theackevent uses the updated callback instead of the original one
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| lib/diameter/src/transport/diameter_tcp.erl | Changes order of operations in actions/3 to process remaining actions before sending message |
| lib/diameter/src/transport/diameter_sctp.erl | Applies the same fix to SCTP transport for consistency |
|
Hi, could you provide the argument why do you think this change is necessary? If I understand the code correctly this could break existing applications. There could be code written in a way that expects ack to land in old callback, and suddenly we will switch it, and possibly break something. |
|
I think one of more valuable use cases of allowing to return a new callback is to support a sort of state. A part for the specific state use case I think normalizing it as suggested would make the API more coherent and easy to be used, the fact that for that specific situation the callback is not immediately changed is counterintuitive and can lead to errors difficult to spot. Please feel free to ask more if the motivation is not sufficiently clear, I can provide an example of where/how it is failing |
Hi, I think you're right, could you rebase your branch from tag 'patch-base-27' and add tests to this branch? I've come up with a testcase that detects this problem here: master...Mikaka27:otp:michal/diameter/call-message-cb-with-updated-callback You can use it, unless you have a better test case that can be used instead, thanks :) |
|
Hi, I've added the tests and changed the target branch. I need to test this on our CI overnight. |
|
Thank you and sorry but it is a very busy period forme |
Diameter
message_cbcan return an updated callback that should be the one to be called for all subsequent events (recv,send,ack). When returning the new callback in response of asendevent the subsequentackevent is notified with the old callback instead of the new one. This happens because the message is sent before parsing all the actions returned by thesendcallback.