[libnice] multithread g_main_loop_quit ad remove agent

[Philip Withnall]

On Mon, 2014-07-14 at 11:25 +0200, Emanuele Bizzarri wrote:
> Hi Philip,
> ok we started working on 0.1.7 plus gstreamer 1.3.90.
> Using examples we find an issue on test-parse.
> stun_message_validate_buffer_length function set 
> stun_message_validate_buffer_length_fast return value to var fast_retval.
> But fast_retval type is unsigned long and 
> stun_message_validate_buffer_length_fast return -1 
> (STUN_MESSAGE_BUFFER_INVALID is -1).
> So (fast_retval<=0) will be always false.
> We have changed fast_retval type (signed long) and now all seems work 
> correctly.

Where *exactly* in the code is this? Looking at my copy of
stun/stunmessage.c, the logs show that fast_retval has always had type
ssize_t (which is signed).

> Il 14/07/2014 09:41, Philip Withnall ha scritto:
>  > Otherwise you could cause a deadlock by calling a libnice function on 
> that agent from the callback, I think. Olivier knows more about that 
> design decision than me though.
> Deadlock shold not happen because if I use a libnice function inside the 
> callback the mutex will be acquired twice from the same thread and 
> released twice at the end of the callback.

libnice uses *non*-recursive mutexes, so acquiring the same mutex twice,
nested, in the same thread will cause deadlock.

libnice previously used recursive mutexes, but we switched because they
are almost impossible to get right in every situation. Using
non-recursive mutexes is a lot more maintainable.

>  > If you want to destroy the user_data in a thread other than the one
>  > running the main context you passed to nice_agent_attach_recv(), you
>  > need to either:
>  >  • implement your own locking on the user_data, or
>  >  • (preferably) make it reference counted, and ensure the reference
>  > counting functions are thread safe.
> 
> Our application is not based on glib, so we are required to destroy 
> user_data from another thread.

You can still implement your own reference counting for the user_data.
If it is a struct, just add a new member:
    volatile int ref_count;
and thread-safe ref and unref functions:
    MyStruct *my_struct_ref (MyStruct *self) {
        g_atomic_int_inc (&self->ref_count);
        return self;
    }
and:
    void my_struct_unref (MyStruct *self) {
        if (g_atomic_int_dec_and_test (&self->ref_count)) {
            /* Free the structure here. */
        }
    }

If needed, you can replace the g_atomic_*() functions with non-GLib
versions with the same behaviour (though I cannot see any advantage in
doing that, given that you are linking with GLib anyway).

Philip
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 213 bytes
Desc: This is a digitally signed message part
URL: <http://lists.freedesktop.org/archives/nice/attachments/20140714/1413a2b4/attachment.sig>