[libnice] Sending packets uses a static, global lock?

Thu Oct 13 09:10:26 UTC 2016

2016-10-13 6:46 GMT+02:00 Anand Sivaram <aspnair at gmail.com>:

> What about having a global flag for the lock type?
> Set it with g_object_set during the library initialization.
> It should have 3 values - Off, Per Agent, Global.
> Modify agent_lock() and agent_unlock() accordingly.
>
> That way if the applications using libnice implemented their own locking
> mechanism, that could be used seamlessly.
>
>

That looks like a smart approach, especially as it would be flexible: the
off part is particularly interesting, as I have locks all over the place
myself! :-). I'm still concerned about whether or not this can result in
issues with the glib2 stuff libnice uses internally, though, and that
applications don't have access to? There may be private methods invoked
that need that locking, something the application from outside may not be
able to protect.

Lorenzo

> anand
>
>
>
>
> On 12 October 2016 at 21:35, Lorenzo Miniero <lminiero at gmail.com> wrote:
>
>> Thanks for the context! (and sorry for the delay in this response...)
>>
>> I'm not familiar enough with glib2, unfortunately: considering the patch
>> dates back to about 7 years ago, is the problem the global muxed fixed
>> still an issue with current versions? We did a quick and dirty test by
>> turning those global agent locks to per-agent locks, where possible (that
>> is, in methods that actually provided a pointer to an agent instance) and
>> we did notice improvements in the performance (about twice as many sessions
>> before it started to break down), but of course this means little if it
>> risks causing those race conditions and crashes again. Is this road worth
>> exploring? I'm willing to help and contribute to such work, of course,
>> although I admit I'm not an expert on the libnice internals (but it's never
>> too late to learn! ;-) ).
>>
>> Thanks,
>> Lorenzo
>>
>>
>> 2016-10-05 13:09 GMT+02:00 Miguel París Díaz <mparisdiaz at gmail.com>:
>>
>>> Sorry for the copy-paste error.
>>>
>>>
>>>
>>> 2016-10-05 13:06 GMT+02:00 Miguel París Díaz <mparisdiaz at gmail.com>:
>>>
>>>> Good point Lorenzo!!!
>>>>
>>>> This change was added in this commit:
>>>>
>>>> commit 2faa76a4840100bebd9532bd47a2755874c5b3a0
>>>> Author: Youness Alaoui <youness.alaoui at collabora.co.uk>
>>>> Date:   Tue Jun 16 20:40:33 2009 -0400
>>>>
>>>>     Use a global mutex for all nice agents and use
>>>> g_source_is_destroyed to avoid the race condition of a source being
>>>> destroyed while we wait for the mutex
>>>>
>>>> And then the RecMutex was changed by a Mutex in:
>>>>
>>>
>>> commit 1deee69325284c726c3a8380a7d5839a51e20c48
>>> Author: Olivier Crête <olivier.crete at collabora.com>
>>> Date:   Mon Feb 24 18:51:31 2014 -0500
>>>
>>>     agent: Replace recursive mutex with non-recursive
>>>
>>>     This should prevent us from re-adding re-entrancy in the future
>>>
>>>
>>>>
>>>>
>>>> 2016-10-05 11:45 GMT+02:00 Lorenzo Miniero <lminiero at gmail.com>:
>>>>
>>>>> Hi all,
>>>>>
>>>>> we've been working on some stress tests of our Janus WebRTC server,
>>>>> which uses libnice for all the ICE related stuff. While it works great in
>>>>> general, we reach a wall much sooner than we expect, and performance start
>>>>> to degrade quite a bit. By doing some digging and diagnostic in the code we
>>>>> wrote and the one we use, we found out something we thought was weird.
>>>>>
>>>>> Specifically, we noticed that the internal method that is used to send
>>>>> packets, nice_agent_send_messages_nonblocking (which nice_agent_send
>>>>> itself uses internally) has a lot of code wrapped in an
>>>>> agent_lock()/agent_unlock(), which locks/unlocks a global mutex, and this
>>>>> includes the process of physiclly sending the packet. This means that the
>>>>> whole library is locked until that code is called, which seems overkill. I
>>>>> guess the same happens in other parts of the code as well. I'd understand
>>>>> locking the specific agent if the purpose is thread-safety, but why lock
>>>>> ALL agents at the same time for each packet you send? I tried searching the
>>>>> archives for some info on that, but couldn't find any discussion related to
>>>>> this choice.
>>>>>
>>>>> Since I'm already handling thread-safety for most stuff in my own
>>>>> code, is there any way for preventing this from happening? I guess that one
>>>>> possible way could be to retrieve the file descriptor from the agent object
>>>>> and send the packet manually myself, but while this may work in simple
>>>>> (host/prflx/srflx) connectivity, this would definitely not be the case
>>>>> when, for instance, TURN is involved, as that's one of the things the
>>>>> library would take care of for me.
>>>>>
>>>>> Looking forward to your thoughts, thanks!
>>>>> Lorenzo
>>>>>
>>>>> _______________________________________________
>>>>> nice mailing list
>>>>> nice at lists.freedesktop.org
>>>>> https://lists.freedesktop.org/mailman/listinfo/nice
>>>>>
>>>>>
>>>>
>>>>
>>>> --
>>>> Miguel París Díaz
>>>> ------------------------------------------------------------
>>>> ------------
>>>> Computer/Software engineer.
>>>> Researcher and architect in http://www.kurento.org
>>>> http://twitter.com/mparisdiaz
>>>> ------------------------------------------------------------
>>>> ------------
>>>>
>>>
>>>
>>>
>>> --
>>> Miguel París Díaz
>>> ------------------------------------------------------------------------
>>> Computer/Software engineer.
>>> Researcher and architect in http://www.kurento.org
>>> http://twitter.com/mparisdiaz
>>> ------------------------------------------------------------------------
>>>
>>
>>
>> _______________________________________________
>> nice mailing list
>> nice at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/nice
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/nice/attachments/20161013/1ce5ef6d/attachment.html>