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.

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

Respond with ACK first before hanging up call for dialog fork #3445

Merged
merged 4 commits into from
Mar 20, 2023

Conversation

sauwming
Copy link
Member

@sauwming sauwming commented Mar 16, 2023

Attempt to fix #3234

Currently for dialog fork, the flow is like this:
sip_ua_layer -> on_dlg_forked() -> pjsua_call: send BYE -> back to sip_ua_layer: respond with ACK.

Ideally, we want the ACK to be sent first and the patch will do this. Unfortunately, it will cause a behaviour change: now it's app's responsibility to send the ACK.

As a consequence of the behaviour change, the sipp script uas-forked-200.xml to test the scenario needs to be modified.

So do you think we should change the current behaviour?

@sauwming sauwming self-assigned this Mar 16, 2023
@sauwming sauwming added this to the release-2.14 milestone Mar 16, 2023
@sauwming
Copy link
Member Author

Committed a new change. Please check the updated PR desc.

@nanangizz
Copy link
Member

  • The change impact can be more than a behavior change, for example, any existing apps that does not call pjsip_dlg_on_rx_response() in its forking callback will break? Also note that pjsip_dlg_on_rx_response() is specified as internal function.
  • Instead of introducing new API pjsip_rdata_set_dlg(), perhaps the steps can be inserted into pjsip_dlg_fork()?
  • Perhaps simply do/move the ACK sending before invoking fork callback (if it is a final response)? But perhaps we need to consider this, IIRC there was request that app can send the ACK manually for some reason.

@sauwming
Copy link
Member Author

I tried another approach to address the above concerns.

So in the latest patch, app (pjsua_call) will manually send ACK inside the callback on_dlg_forked().

Pros:

  • No modification within the pjsip component.
  • No break if app doesn't send ACK in on_dlg_forked().

Con:

  • ACK will be sent twice, once by app inside on_dlg_forked(), another by pjsip_dlg_on_rx_response(). This shouldn't cause any harm though, and it still passes the sipp test. If this is unwanted, I can perhaps introduce a boolean variable in sip_dialog to check if ack is already sent.

Let me know what you think.

@sauwming
Copy link
Member Author

The last commit is to address the above con. Now ACK is only sent once.

Copy link
Member

@nanangizz nanangizz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea, a safer approach.

@sauwming sauwming merged commit 15902a8 into master Mar 20, 2023
@sauwming sauwming deleted the dlg-fork branch March 20, 2023 01:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Response to Call forking not correct
3 participants