-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Optional forcing of private IDs for subscriptions for better kick in VideoRoom #856
Conversation
Hi, I tested this, it's work.. so listener dropped correctly, but after user kicked and kicked user trying to reconnect, janus crashed. Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7fffd4bd6700 (LWP 28004)]
0x00007ffff714f2e5 in g_mutex_lock () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
#0 0x00007ffff714f2e5 in g_mutex_lock () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
#1 0x00007ffff70dfd7b in g_async_queue_push () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
#2 0x000000000042b394 in janus_ice_webrtc_hangup (handle=handle@entry=0x7fff94002f90) at ice.c:1169
#3 0x0000000000439d54 in janus_plugin_close_pc (plugin_session=<optimized out>) at janus.c:3078
#4 0x00007fffec01e77a in janus_videoroom_handle_message (handle=0x7fff6c001410, transaction=0x7fffc00048e0 "aZyTm4YNALNb", message=0x7fff64007c70, jsep=0x0) at plugins/janus_videoroom.c:1959
#5 0x0000000000440286 in janus_process_incoming_request (request=0x7fff64001900) at janus.c:1339
#6 0x00000000004444b5 in janus_transport_task (data=0x7fff64001900, user_data=<optimized out>) at janus.c:2582
#7 0x00007ffff71321d8 in ?? () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
#8 0x00007ffff7131845 in ?? () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
#9 0x00007ffff59f2064 in start_thread (arg=0x7fffd4bd6700) at pthread_create.c:309
#10 0x00007ffff572762d in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:111 |
Ack, thanks for testing, I'll look into this. |
Can't seem to be able to replicate this: what do you mean by reconnecting the kicked publisher? If I try a new |
Nevermind that, got it to crash... |
#0 0x00007ffff7126b28 in ?? () from /lib/x86_64-linux-gnu/libglib-2.0.so.0 |
Can you check if this fixes it for you? Notice that this will not close the listeners' PeerConnections, but it will mark them as kicked and no media will flow any more, meaning they'll have to close them down as they'll be useless from now on. Trying to close them from within the kick code is what triggered the race condition and the crash for me, as the |
Yes, it's working without crash |
Have you done some more tests? Wondering if it's safe to merge now. |
Nothing else? Well, looks good to me, so merging. |
This PR addresses the discussion we had in #745.
The motivation comes from the fact that, when you kick a user, normally their subscriptions still live in the room and they keep on getting media: that's expected as we kick a publisher, but listeners don't necessarily have a context, as we don't mandate them to be associated to any publisher at all.
This PR allows for this association to be optionally mandated, based on the
private_id
property we introduced recently. The idea is the following:private_id
by the plugin: you're not supposed to share this.private_id
as part of the request, to allow the plugin to know this was originated by you. The VideoRoom demo already does that. Anyway, this parameter is entirely optional, for reasons explained in ACL and kick support in AudioBridge, VideoRoom and TextRoom #745 (we still want to support listeners that are created out of context or with no associated publisher handle present).require_pvtid:true
, then theprivate_id
part when subscribing is mandatory, and it must match one of the existing publishers: if it's missing, or it's invalid, the subscription fails.private_id
, and so close all listeners with the sameprivate_id
too. Of course, this is only guaranteed to succeed ifrequire_pvtid:true
, as otherwise nothing prevents listeners to be created without aprivate_id
.Tested briefly with three participants and seems to work. Please test carefully to check if it works and if it doesn't introduce any regression or problem/crash.