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 simulated leave message for longer string ID rooms #3243

Merged
merged 1 commit into from
Aug 4, 2023

Conversation

adnanel
Copy link
Contributor

@adnanel adnanel commented Jul 13, 2023

Currently, on abrupt disconnections, participants get stuck in text rooms and aren't able to re-join immediately.

This makes auto reconnect mechanisms needlessly complicated.

In this PR I added automatic leaving of all rooms when the session is being destroyed.

The bug can be reproduced in the official janus textroom demo:

  • Join textroom via phone
  • Join textroom via PC
  • Disconnect internet on phone
  • On PC you can still see this participant for

@adnanel adnanel force-pushed the textroom-leave branch 2 times, most recently from be501b4 to 6e544e0 Compare July 13, 2023 13:08
@lminiero
Copy link
Member

Thanks for the patch, but I think you're doing those things in the wrong place. You've added some logic to the destroy_session callack, but it's actually the hangup_media_internal one where those things should be done, since once the PeerConnection is gone (whether the session exists or not) then all relationships to room resources are gone. As a matter of fact, we have code for that already:

https://github.com/meetecho/janus-gateway/blob/master/src/plugins/janus_textroom.c#L3009-L3043

which is supposed to do exactly that, that is leaving rooms you're in and destroying the participants resources. If you think that's not working as expected for some reason, please check what needs to be fixed there instead.

@lminiero lminiero added the multistream Related to Janus 1.x label Jul 13, 2023
@lminiero
Copy link
Member

lminiero commented Jul 13, 2023

I just tried to replicate your steps, and it seems to work just fine in my case. Obviously the user only leaves once the session timeout kicks in, so ~60s later by default, but the same thing would happen with your patch too, since you're intercepting destroy_session which is triggered by a handle detach, so only once the session timeout gets rid of all related handles.

@adnanel
Copy link
Contributor Author

adnanel commented Jul 13, 2023

If what you're saying is correct (I'm already finishing for the day so can't confirm), the bug I'm trying to fix isn't the same issue I found on the demo, so I should clarify our original problem, then:

We are seeing participants in text rooms well over 60 seconds (instead, they are permanently stuck).
The handle and session are both destroyed, but the other participants still see them and they are no longer able to join.

It only works if they properly perform "leave" before. Also the version from this branch seems to also fix our problem.

I will try to analyze it further and see what actually happens.

@lminiero
Copy link
Member

Then yes, the problem may be in the code I linked above, which is where the logic for "let's remove the user" is, so that's where I'd dig: I couldn't replicate the problem, as the session timeout cleaned the user correctly for me. It may have something to do with the "fake" message we inject to simulate a "leave" message from the user: my guess is that you do end up calling that code, and the user is removed, but the event is not propagated as it should because of something that goes wrong with that "fake" message.

@adnanel
Copy link
Contributor Author

adnanel commented Jul 24, 2023

Hi @lminiero

I had time to work on this again, turned out your leave command failed for us due to being an invalid JSON, which in turn is invalid due to our (string) room ID being too long (it's an UUID with a suffix).

I changed the PR to a much simpler version which only expands the request string buffer to 200 characters. Will test today and confirm whether this fixes everything.

Edit: Yep, seems to work now...

@adnanel adnanel changed the title Leave all text rooms when session is being destroyed Fix simulated leave message for longer string ID rooms Jul 24, 2023
@lminiero
Copy link
Member

lminiero commented Aug 4, 2023

@adnanel apologies for the very late response, I've been abroad for the IETF meeting and I'm catching up with all the pending activities. So it was just a matter of the buffer for the "fake" message being too short? If that's the case, it's indeed a very easy fix and I'll merge!

@lminiero lminiero merged commit 8592559 into meetecho:master Aug 4, 2023
8 checks passed
mwalbeck pushed a commit to mwalbeck/docker-janus-gateway that referenced this pull request Dec 8, 2023
This PR contains the following updates:

| Package | Update | Change |
|---|---|---|
| [meetecho/janus-gateway](https://github.com/meetecho/janus-gateway) | minor | `v1.1.4` -> `v1.2.1` |

---

### Release Notes

<details>
<summary>meetecho/janus-gateway (meetecho/janus-gateway)</summary>

### [`v1.2.1`](https://github.com/meetecho/janus-gateway/blob/HEAD/CHANGELOG.md#v121---2023-12-06)

[Compare Source](meetecho/janus-gateway@v1.2.0...v1.2.1)

-   Added support for abs-capture-time RTP extension \[[PR-3161](meetecho/janus-gateway#3161)]
-   Fixed truncated label in datachannels (thanks [@&#8203;veeting](https://github.com/veeting)!) \[[PR-3265](meetecho/janus-gateway#3265)]
-   Support larger values for SDP attributes (thanks [@&#8203;petarminchev](https://github.com/petarminchev)!) \[[PR-3282](meetecho/janus-gateway#3282)]
-   Fixed rare crash in VideoRoom plugin (thanks [@&#8203;tmatth](https://github.com/tmatth)!) \[[PR-3259](meetecho/janus-gateway#3259)]
-   Don't create VideoRoom subscriptions to publisher streams with no associated codecs
-   Added suspend/resume participant API to AudioBridge \[[PR-3301](meetecho/janus-gateway#3301)]
-   Fixed rare crash in AudioBridge
-   Fixed rare crash in Streaming plugin when doing ICE restarts \[[Issue-3288](meetecho/janus-gateway#3288)]
-   Allow SIP and NoSIP plugins to bind media to a specific address (thanks [@&#8203;pawnnail](https://github.com/pawnnail)!) \[[PR-3263](meetecho/janus-gateway#3263)]
-   Removed advertised support for SIP UPDATE in SIP plugin
-   Parse RFC2833 DTMF to custom events in SIP plugin (thanks [@&#8203;ywmoyue](https://github.com/ywmoyue)!) \[[PR-3280](meetecho/janus-gateway#3280)]
-   Fixed missing Contact header in some dialogs in SIP plugin (thanks [@&#8203;ycherniavskyi](https://github.com/ycherniavskyi)!) \[[PR-3286](meetecho/janus-gateway#3286)]
-   Properly set mid when notifying about ended tracks in janus.js
-   Fixed broken restamping in janus-pp-rec
-   Other smaller fixes and improvements (thanks to all who contributed pull requests and reported issues!)

### [`v1.2.0`](https://github.com/meetecho/janus-gateway/blob/HEAD/CHANGELOG.md#v120---2023-08-09)

[Compare Source](meetecho/janus-gateway@v1.1.4...v1.2.0)

-   Added support for VP9/AV1 simulcast, and fixed broken AV1/SVC support \[[PR-3218](meetecho/janus-gateway#3218)]
-   Fixed RTCP out quality stats \[[PR-3228](meetecho/janus-gateway#3228)]
-   Default link quality stats to 100
-   Added support for ICE consent freshness \[[PR-3234](meetecho/janus-gateway#3234)]
-   Fixed rare race condition in VideoRoom \[[PR-3219](meetecho/janus-gateway#3219)] \[[PR-3247](meetecho/janus-gateway#3247)]
-   Use speexdsp's jitter buffer in the AudioBridge \[[PR-3233](meetecho/janus-gateway#3233)]
-   Fixed crash in Streaming plugin on mountpoints with too many streams \[[Issue-3225](meetecho/janus-gateway#3225)]
-   Support for batched configure requests in Streaming plugin (thanks [@&#8203;petarminchev](https://github.com/petarminchev)!) \[[PR-3239](meetecho/janus-gateway#3239)]
-   Fixed rare deadlock in Streamin plugin \[[PR-3250](meetecho/janus-gateway#3250)]
-   Fix simulated leave message for longer string ID rooms in TextRoom (thanks [@&#8203;adnanel](https://github.com/adnanel)!) [PR-3243](meetecho/janus-gateway#3243)]
-   Notify about count of sessions, handles and PeerConnections on a regular basis, when event handlers are enabled \[[PR-3221](meetecho/janus-gateway#3221)]
-   Fixed broken Insertable Streams for recvonly streams when answering in janus.js
-   Added background selector and blur support to Virtual Background demo
-   Other smaller fixes and improvements (thanks to all who contributed pull requests and reported issues!)

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNi4zNC4wIiwidXBkYXRlZEluVmVyIjoiMzcuODEuNCIsInRhcmdldEJyYW5jaCI6Im1hc3RlciJ9-->

Reviewed-on: https://git.walbeck.it/walbeck-it/docker-janus-gateway/pulls/122
Co-authored-by: renovate-bot <bot@walbeck.it>
Co-committed-by: renovate-bot <bot@walbeck.it>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
multistream Related to Janus 1.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants