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

Fix hangup on inviting status after #2521 #2536

Closed
wants to merge 1 commit into from
Closed

Fix hangup on inviting status after #2521 #2536

wants to merge 1 commit into from

Conversation

AdrianoMartins
Copy link

The merge #2521 is causing a problem in the hangup of the call with status inviting, similar to the problem reported in #1856. I believe that the correct treatment for the situation of #2521, is to do with status different from destroyed.

@januscla
Copy link

Thanks for your contribution, @AdrianoMartins! Please make sure you sign our CLA, as it's a required step before we can merge this.

@lminiero
Copy link
Member

lminiero commented Jan 27, 2021

Pinging @zayim as he contributed the patch you mentioned, so that you can both check if this fixes your requirements.

@zayim
Copy link
Contributor

zayim commented Jan 28, 2021

Hi! Sorry for late response.

I agree that merge #2521 broke hangup of inviting calls. But change you suggested just reverts my PR, since if you changed will always be true (due to lines 2528-2529).

I suggest adding additional check in if, so we send BYE/CANCEL only if session is established or status is inviting. In all other cases, we would send SIP response instead of BYE, which is correct behaviour.

Something like this: master...zayim:fix-sip-plugin-bye-on-inviting

I reproduced error on inviting calls on master, tested this code from my branch and verified that it fixes it.

@lminiero
Copy link
Member

lminiero commented Feb 1, 2021

@zayim I think you moved it too much ahead, since it within the mutex lock now. I can't remember if there were risks of deadlocks there, but if the main purpose is doing something before the state changes, maybe it's simpler to move it before the janus_sip_call_update_status call?

@lminiero
Copy link
Member

lminiero commented Feb 4, 2021

@AdrianoMartins ping 🙂

@zayim
Copy link
Contributor

zayim commented Feb 4, 2021

@lminiero I agree, this branch was in a rush just to test my theory. If Adriano agrees, I can edit by branch per your suggestion and make PR.

@lminiero
Copy link
Member

lminiero commented Feb 4, 2021

@zayim sounds good to me, I'll wait for you guys to agree on which solution works best for you both.

@AdrianoMartins
Copy link
Author

Sorry for the late response. @zayim I agree. Thanks.

@lminiero
Copy link
Member

lminiero commented Feb 9, 2021

@AdrianoMartins thanks for the feedback!

@zayim
Copy link
Contributor

zayim commented Feb 9, 2021

Great, I will submit PR in next few hours.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants