Close nice agent resources asynchronously #2492

fbellet · 2020-12-14T18:49:46Z

This merge request uses the function nice_agent_close_async() to free the remote reservations that the nice agent may have on remote turn servers, before being disposed. This typically adds a delay of one or two seconds before disposing the agent (one stun request, and two retransmissions, since these stun requests are not acked).

I tested with both static event loops and per-thread event loop.

januscla · 2020-12-14T18:49:48Z

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

lminiero · 2020-12-14T19:05:28Z

Thanks @fbellet! I'll review tomorrow, pinging @atoppi so that he can check the changes too.

tmatth · 2020-12-14T19:08:13Z

ice.c

+		if(G_IS_OBJECT(handle->agent)) {
+			JANUS_LOG(LOG_INFO, "[%"SCNu64"] Closing nice agent %p\n", handle->handle_id, handle->agent);
+			nice_agent_close_async(handle->agent, janus_ice_cb_agent_closed, handle);
+			janus_refcount_increase(&handle->ref);


Would it make sense to increase the refcount before the nice_agent_close_async call (to avoid a race), or are we guaranteed that janus_ice_cb_agent_closed will not be called before the end of this block (and thus, there is no race)?

The handle is already protected by a mutex.

Edit: sorry, answering late and I'm being silly... anyway, janus_ice_cb_agent_closed is executed by the glib loop, which means that if the loop is running, then the handle definitely has a reference still up (definitely the case for single-thread-per-loop, I should check if it's the same for event loops too but I think so).

lminiero

@fbellet I just checked the code and I think there are some wrong assumptions. You apparently exit the loop when the agent is closed, which should not happen: the loop is created when the handle is attached, and is only stopped when the handle itself is detached/destroyed. The confusion may derive from the fact that the source handling the loop is (pobably improperly) called janus_ice_outgoing_traffic, but it doesn't handle just that: it actually handles other states as well. The same handle may have multiple PeerConnections in its life cycle, not just one.

As such, the way the patch works right now, unless static event loops are used the handle would be rendered useless as soon as the first PeerConnection created on it is destroyed, since the loop would disappear (and so would the handle thread).

This also means that preventing janus_ice_outgoing_traffic_finalize from closing loop is also probably incorrect. I realize it's done to avoid the last janus_ice_cb_agent_closed from being called, though (if we're closing the agent because the whole handle was detached, for instance), so there may need to be a more complex logic there.

I also added an inline note on nice_agent_attach_recv.

lminiero · 2020-12-16T10:00:51Z

ice.c

@@ -1431,14 +1447,25 @@ static void janus_ice_webrtc_free(janus_ice_handle *handle) {
 		return;
 	}
 	handle->agent_created = 0;
+#ifdef HAVE_CLOSE_ASYNC
+	nice_agent_attach_recv(handle->agent, handle->stream_id, 1, g_main_loop_get_context(handle->mainloop), NULL, NULL);
+#endif


We actually stopped doing this in #2101, because of this issue in libnice that is apparently still open. As such, I'd rather not reintroduce it here, unless you know it can't cause the issue presented above.

atoppi

I agree with @lminiero.
The janus_ice_cb_agent_closed callback should not be in charge of quitting the loop because an agent lifecycle is always shorther than an handle lifecycle.
Agents may be destroyed and then recreated any time a new ICE local setup starts in janus_ice_setup_local.

fbellet · 2020-12-18T15:39:42Z

Thank you for the review. I'll update my pull request to take your comments into account:

by adding a ref on handle->rtp_source to prevent the GSource from calling its finalize callback janus_ice_outgoing_traffic_finalize (and from quitting the event loop) before the nice agent is ready to be closed.
by removing the agent stream instead of setting a null callback in nice_agent_attach_recv

atoppi

Now the PR looks good to me 👍
I've made some tests with janus local turn candidates and it seems to work as expected.

lminiero

Apologies if this took a while, I've only recently come back to work after the holidays. I agree the patch looks fine (haven't tested it yet, I will later today), I just added a couple of notes on the possibly excessive verbosity of the new log lines.

lminiero · 2021-01-13T11:17:17Z

ice.c

+	janus_ice_outgoing_traffic *t = (janus_ice_outgoing_traffic *)data;
+	janus_ice_handle *handle = t->handle;
+
+	JANUS_LOG(LOG_INFO, "[%"SCNu64"] Disposing nice agent %p\n", handle->handle_id, handle->agent);


This should probably be a LOG_VERB, rather than LOG_INFO.

lminiero · 2021-01-13T11:17:39Z

ice.c

+#ifdef HAVE_CLOSE_ASYNC
+		if(G_IS_OBJECT(handle->agent)) {
+			JANUS_LOG(LOG_INFO, "[%"SCNu64"] Removing stream %d from agent %p\n",
+				handle->handle_id, handle->stream_id, handle->agent);


Same here, we don't really need all that info displayed by default, while it's useful in verbose mode.

ice.c

lminiero · 2021-01-20T10:28:12Z

Thanks for the fixes and once more for the contribution, merging! 👍

tgabi333 · 2021-01-20T14:01:33Z

is it a performance related PR?

tmatth · 2021-01-26T20:14:13Z

is it a performance related PR?

AFAICT the motivation here is to not leave TURN allocations lingering on the remote TURN server, see:
https://libnice.freedesktop.org/libnice/NiceAgent.html#nice-agent-close-async

Otherwise e.g., coturn has to clean them up itself after some timeout has passed.

commit caaba91 Author: Tijmen de Mes <tijmen@ag-projects.com> Date: Tue Feb 23 14:57:17 2021 +0100 Added Content type to SIP message (meetecho#2567) * Added 'content_type' to received SIP MESSAGE * Added optional content type in sending SIP MESSAGE commit c9baba9 Author: Alessandro Toppi <atoppi@meetecho.com> Date: Tue Feb 23 11:46:50 2021 +0100 clang/ubsan fixes (meetecho#2556) * Fix some clang warnings. * Fix UBSanitizer error when sending RTCP SR. commit beb28be Author: Tvildo <davidtamuna@gmail.com> Date: Mon Feb 22 09:46:25 2021 -0800 add call_id in received sip message (meetecho#2563) Add call_id in received SIP MESSAGE and INFO commit 8246452 Author: Lorenzo Miniero <lminiero@gmail.com> Date: Mon Feb 22 11:51:36 2021 +0100 Fixed missing mutexes around VideoRoom ACL management commit 4f8943a Author: Tristan Matthews <tmatth@videolan.org> Date: Wed Feb 17 09:32:57 2021 -0500 ice: fix conncheck typo (meetecho#2560) No functional change since the typo was used consistently. commit 27dc51a Author: nicolasduteil <nduteil@freedev.org> Date: Wed Feb 17 15:27:45 2021 +0100 feat: add "call_id" to "calling", "declining", "updatingcall" & "incomingcall" events (meetecho#2557) commit 2c81d02 Author: Hritik Utekar <hritikdrocks@gmail.com> Date: Wed Feb 17 19:54:46 2021 +0530 Video moderation always returns unmuted (meetecho#2559) commit 6503f42 Author: Lorenzo Miniero <lminiero@gmail.com> Date: Wed Feb 17 13:53:53 2021 +0100 Fixed typo in keepalive-conncheck usage commit 1f45e02 Author: Alessandro Toppi <atoppi@meetecho.com> Date: Mon Feb 15 16:38:22 2021 +0100 Set specific versions for Python 3 and meson in janus-ci yml. commit d7c9ef0 Author: Lorenzo Miniero <lminiero@gmail.com> Date: Mon Feb 15 15:28:48 2021 +0100 Added audiocodec/videocodec supporto to 'joinandconfigure' in VideoRoom API commit ad54495 Author: Lorenzo Miniero <lminiero@gmail.com> Date: Fri Feb 12 15:28:21 2021 +0100 Add new option to configure ICE nomination mode, if libnice is recent enough (meetecho#2541) * Add new option to configure ICE nomination mode, if libnice is recent enough * Added support for libnice keepalive-conncheck property commit af8cc6e Author: Nadin Zajimovic <zayim92@gmail.com> Date: Fri Feb 12 09:40:36 2021 +0100 if inviting on destroy, send BYE instead of 480 response (meetecho#2554) commit ad8bf79 Author: Alessandro Amirante <alex@meetecho.com> Date: Thu Feb 11 17:49:25 2021 +0100 Fix typo in videoroom docs. commit 26f5958 Author: Lorenzo Miniero <lminiero@gmail.com> Date: Wed Feb 10 16:51:19 2021 +0100 Fixed small leak in VideoRoom commit 8ab7a00 Author: Alessandro Toppi <atoppi@meetecho.com> Date: Tue Feb 9 16:51:37 2021 +0100 Initialize packet.is_rtp to false. commit 66cf343 Author: Lorenzo Miniero <lminiero@gmail.com> Date: Tue Feb 9 16:05:37 2021 +0100 Add resolution and bitrate to Record&Play playback commit 119d220 Author: Aleksander Guryanov <caiiiycuk@gmail.com> Date: Tue Feb 9 20:33:23 2021 +0700 Update janus.d.ts (meetecho#2553) Function getBitrate() actually returns a string commit 41399db Author: Lorenzo Miniero <lminiero@gmail.com> Date: Mon Feb 8 16:27:14 2021 +0100 Allow up to 5 (rather than 3) audio/video codecs in the same VideoRoom commit b81dd6d Author: Lorenzo Miniero <lminiero@gmail.com> Date: Mon Feb 8 16:26:19 2021 +0100 Allow forcing audio/video codec for VideoRoom publishers via query string commit 576abf5 Author: Lorenzo Miniero <lminiero@gmail.com> Date: Mon Feb 8 15:17:56 2021 +0100 Initialize VideoRoom participant recording state when room recording is active (fixes meetecho#2550) commit 0ba74fb Author: Lorenzo Miniero <lminiero@gmail.com> Date: Mon Feb 8 11:53:42 2021 +0100 Fixed broken AV1 post-processing commit 09daec4 Author: Lorenzo Miniero <lminiero@gmail.com> Date: Mon Feb 8 10:46:06 2021 +0100 Renamed extern janus_callbacks variables in Lua and Duktape plugins (meetecho#2540) commit 664022b Author: Lorenzo Miniero <lminiero@gmail.com> Date: Mon Feb 8 10:41:38 2021 +0100 Bumped to version 0.11.1 commit 7732127 Author: Lorenzo Miniero <lminiero@gmail.com> Date: Mon Feb 8 10:37:41 2021 +0100 Updated Changelog (0.10.10) commit 24a0eec Author: Lorenzo Miniero <lminiero@gmail.com> Date: Thu Feb 4 11:53:36 2021 +0100 Videoroom race condition fixes (see meetecho#2509) (meetecho#2539) * Fixed missing room references that could cause crashes during race conditions * Fixed rare race condition on publisher join commit 62440c5 Author: Lorenzo Miniero <lminiero@gmail.com> Date: Thu Feb 4 11:52:44 2021 +0100 Fix parsing of SDP to find payload type matching profiles (fixes meetecho#2544) (meetecho#2549) commit 794e89a Author: Bender <undead-empire@ya.ru> Date: Wed Feb 3 20:12:02 2021 +0300 janus.js (meetecho#2548) customizeSdp callback added to handleRemoteJsep to be able to mangle remote SDP if needed commit 213b6c7 Author: Alessandro Toppi <atoppi@meetecho.com> Date: Fri Jan 29 12:13:44 2021 +0100 Make compiler fail if implicit-function-declaration is encountered. commit dfa8016 Author: Lorenzo Miniero <lminiero@gmail.com> Date: Fri Jan 29 10:21:50 2021 +0100 Fixed non-portable call to strlcpy, and comment styles, in RabbitMQ code (see meetecho#2430) commit b7b1e9e Merge: 19ecf48 c0f0e1e Author: Alessandro Toppi <atoppi@meetecho.com> Date: Fri Jan 29 08:02:18 2021 +0100 Merge pull request meetecho#2430 from vgrid/master Updates RabbitMQ logic commit 19ecf48 Author: Lorenzo Miniero <lminiero@gmail.com> Date: Thu Jan 28 11:54:55 2021 +0100 Fixed VideoRoom docs on ICE Restarts for subscribers (fixes meetecho#2537) commit 2454802 Author: Lorenzo Miniero <lminiero@gmail.com> Date: Wed Jan 27 11:22:13 2021 +0100 Allow marking of RTP extensions in MJR recordings (meetecho#2527) commit 0bb49bc Author: Lorenzo Miniero <lminiero@gmail.com> Date: Wed Jan 27 11:21:09 2021 +0100 Moderator based muting/unmuting of VideoRoom streams (meetecho#2513) commit 5e685e3 Author: Lorenzo Miniero <lminiero@gmail.com> Date: Wed Jan 27 11:19:35 2021 +0100 Reject a=extmap-allow-mixed in SDP, when offered commit c0f0e1e Author: Chris Wiggins <chris@wiggins.nz> Date: Wed Jan 27 09:59:02 2021 +1300 Fix code style comments, also enable routing for direct exchanges commit 257eb80 Author: Lorenzo Miniero <lminiero@gmail.com> Date: Tue Jan 26 15:00:39 2021 +0100 Configurable media direction when putting calls on-hold (SIP plugin) (meetecho#2525) commit 7fb08c2 Author: Lorenzo Miniero <lminiero@gmail.com> Date: Tue Jan 26 12:40:26 2021 +0100 Added starting DTLS MTU to info returned by Janus API commit 4d97028 Author: Sami Kuhmonen <sami@tokavuh.com> Date: Tue Jan 26 12:53:14 2021 +0200 Report fail if binding to a socket fails in websockets (meetecho#2534) commit 674367a Author: Evgeniy Baranov <jeck.ru@gmail.com> Date: Mon Jan 25 12:00:37 2021 +0300 fix race condition in audiobridge plugin changeroom request (meetecho#2535) commit 3edb780 Author: Alberto Gonzalez Trastoy <albertogontras@gmail.com> Date: Sat Jan 23 14:01:27 2021 -0500 Janus npm types upgrade (meetecho#2528) commit 78434aa Author: August Black <augustblack@gmail.com> Date: Sat Jan 23 12:00:52 2021 -0700 set webrtc-adapter verstion to 7.4.0 (meetecho#2531) commit 46a6c71 Author: Lorenzo Miniero <lminiero@gmail.com> Date: Thu Jan 21 13:02:06 2021 +0100 Reduced verbosity of a few LOG_WARN messages at startup commit 34f6f89 Author: Andrew Lavrentev <andrew.lavrentev@gmail.com> Date: Thu Jan 21 14:48:20 2021 +0300 Feature/enhance typings (meetecho#2518) commit 16173af Author: Rémi Vansteelandt <remvst@gmail.com> Date: Thu Jan 21 05:39:17 2021 -0500 Fixed secret authentication on GET requests (meetecho#2524) commit 62d75ab Author: Nadin Zajimovic <zayim92@gmail.com> Date: Wed Jan 20 11:36:43 2021 +0100 Dont send bye on early dialog (meetecho#2521) commit 2141d9b Author: Yurii Cherniavskyi <yurii.cherniavskyi@gmail.com> Date: Wed Jan 20 12:30:30 2021 +0200 Update Webpack instruction after webrtc-adapter dependency update (meetecho#2519) commit f994f7c Author: fbellet <fabrice@bellet.info> Date: Wed Jan 20 11:28:17 2021 +0100 Close nice agent resources asynchronously (meetecho#2492) commit 79038e0 Author: Sergey Radionov <RSATom@gmail.com> Date: Tue Jan 19 18:16:03 2021 +0700 mqttevh: tls support implementation finished (meetecho#2517) * mqttevh: tls support implementation finished * mqttevh: MQTTASYNC_OPERATION_INCOMPLETE is not error * mqttevh: allow send messages while connecting is still in progress commit 97cd054 Author: Lorenzo Miniero <lminiero@gmail.com> Date: Mon Jan 18 11:24:48 2021 +0100 Fixed broken webrtc-adapter links (see meetecho#2515) commit c0570a9 Author: Tristan Matthews <tmatth@videolan.org> Date: Thu Jan 14 13:24:48 2021 -0500 html: update webrtc-adapter to 7.7.0 (meetecho#2515) commit f57215a Author: Lorenzo Miniero <lminiero@gmail.com> Date: Thu Jan 14 10:11:57 2021 +0100 Updated year in demos and docs commit bbdd3e4 Author: Chris Wiggins <chris@wiggins.nz> Date: Tue Nov 17 11:49:42 2020 +1300 Adds back in default outgoing queue behaviour. Adds support for auto-generated queue_names commit ed1b5c6 Author: Chris Wiggins <chris@wiggins.nz> Date: Thu Nov 12 13:21:08 2020 +1300 Adds RabbitMQ options for queues, durable, exclusive and autodelete commit 24594f7 Author: Chris Wiggins <chris@wiggins.nz> Date: Wed Nov 11 18:05:24 2020 +1300 Check RabbitMQ admin topic in a better way commit 319c6fc Author: Chris Wiggins <chris@wiggins.nz> Date: Wed Nov 11 16:09:26 2020 +1300 Increase RabbitMQ logging on publish commit 505eeef Author: Chris Wiggins <chris@wiggins.nz> Date: Tue Nov 10 18:29:59 2020 +1300 Fix queue_name_admin in rabbitmq transport commit b3f7ad9 Author: Chris Wiggins <chris@wiggins.nz> Date: Tue Nov 10 18:19:07 2020 +1300 Update rabbitmq logging information commit f604aeb Author: Chris Wiggins <chris@wiggins.nz> Date: Tue Nov 10 17:23:11 2020 +1300 Updates RabbitMQ logic - Publishing to a topic does not require an outgoing queue, just the topic, so the outgoing queues are no longer declared - When the janus_exchange_type is topic, we want to be able to name the queue, and then bind an incoming topic from the exchange to that queue, so that functionality has been added - This is all backwards compatible with original logic, and won't break existing logic

tmatth reviewed Dec 14, 2020

View reviewed changes

lminiero reviewed Dec 16, 2020

View reviewed changes

atoppi reviewed Dec 18, 2020

View reviewed changes

fbellet force-pushed the close-nice-agent-resources-asynchronously branch from fca1209 to fddbfea Compare December 18, 2020 15:43

atoppi reviewed Dec 21, 2020

View reviewed changes

lminiero reviewed Jan 13, 2021

View reviewed changes

Close nice agent resources asynchronously

acdd88a

fbellet force-pushed the close-nice-agent-resources-asynchronously branch from fddbfea to acdd88a Compare January 18, 2021 15:33

lminiero merged commit f994f7c into meetecho:master Jan 20, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Close nice agent resources asynchronously #2492

Close nice agent resources asynchronously #2492

fbellet commented Dec 14, 2020

januscla commented Dec 14, 2020

lminiero commented Dec 14, 2020

tmatth Dec 14, 2020

lminiero Dec 14, 2020 •

edited

lminiero left a comment •

edited

lminiero Dec 16, 2020

atoppi left a comment

fbellet commented Dec 18, 2020 •

edited

atoppi left a comment

lminiero left a comment

lminiero Jan 13, 2021

lminiero Jan 13, 2021

lminiero commented Jan 20, 2021

tgabi333 commented Jan 20, 2021

tmatth commented Jan 26, 2021 •

edited

Close nice agent resources asynchronously #2492

Close nice agent resources asynchronously #2492

Conversation

fbellet commented Dec 14, 2020

januscla commented Dec 14, 2020

lminiero commented Dec 14, 2020

tmatth Dec 14, 2020

Choose a reason for hiding this comment

lminiero Dec 14, 2020 • edited

Choose a reason for hiding this comment

lminiero left a comment • edited

Choose a reason for hiding this comment

lminiero Dec 16, 2020

Choose a reason for hiding this comment

atoppi left a comment

Choose a reason for hiding this comment

fbellet commented Dec 18, 2020 • edited

atoppi left a comment

Choose a reason for hiding this comment

lminiero left a comment

Choose a reason for hiding this comment

lminiero Jan 13, 2021

Choose a reason for hiding this comment

lminiero Jan 13, 2021

Choose a reason for hiding this comment

lminiero commented Jan 20, 2021

tgabi333 commented Jan 20, 2021

tmatth commented Jan 26, 2021 • edited

lminiero Dec 14, 2020 •

edited

lminiero left a comment •

edited

fbellet commented Dec 18, 2020 •

edited

tmatth commented Jan 26, 2021 •

edited