After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 762216 - rtpsession: don't lock while emitting the stats signal
rtpsession: don't lock while emitting the stats signal
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
git master
Other All
: Normal major
: 1.9.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2016-02-17 19:09 UTC by Håvard Graff (hgr)
Modified: 2016-05-20 06:26 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
test and fix (4.32 KB, patch)
2016-02-17 19:09 UTC, Håvard Graff (hgr)
needs-work Details | Review
rtpsession: Add test for locking of the stats signal (3.13 KB, patch)
2016-05-15 09:35 UTC, Sebastian Dröge (slomo)
needs-work Details | Review
rtpsession: Add test for locking of the stats signal (2.45 KB, patch)
2016-05-16 07:59 UTC, Mikhail Fludkov
committed Details | Review

Description Håvard Graff (hgr) 2016-02-17 19:09:41 UTC
Created attachment 321537 [details] [review]
test and fix

Keeping the lock while emitting the stats signal introduces potential
deadlock in those situations when the signal callback wants the access
to rtpsession's properties which also requre the lock.
Comment 1 Sebastian Dröge (slomo) 2016-02-18 08:10:42 UTC
Similar issue to bug #762135 btw, might make sense to do a general review of the signalling in rtpbin and related code.
Comment 2 Sebastian Dröge (slomo) 2016-05-15 09:33:10 UTC
This was fixed independently without test now, sorry!

commit fe34f46f32db15b62b445e82d3c7191f682ddf8b
Author: Sebastian Dröge <sebastian@centricular.com>
Date:   Sun May 15 12:30:50 2016 +0300

    rtpsession: Take the lock already when reading the other stats, not just for the hash table
    
    https://bugzilla.gnome.org/show_bug.cgi?id=766025

commit 204a86af97d039ddc71cc61bb8bf8a30827c1db1
Author: Sebastian Dröge <sebastian@centricular.com>
Date:   Wed May 11 09:28:13 2016 +0300

    rtpsession: Don't notify about stats property changes while taking the session lock
    
    The signal handlers might want to actually get the value of the stats
    property, which would take the session lock again and deadlock.
    
    This was introduced by 2e960e70750a0cb7e1117d0c09d08597866a29ee.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=766025

commit 2e960e70750a0cb7e1117d0c09d08597866a29ee
Author: Miguel París Díaz <mparisdiaz@gmail.com>
Date:   Thu May 5 14:18:21 2016 +0200

    rtpsession: Take session lock when creating stats
    
    The access to the session hash table must happen while the session lock is
    taken, otherwise another thread might modify the hash table while we're
    creating the stats.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=766025
Comment 3 Sebastian Dröge (slomo) 2016-05-15 09:35:05 UTC
Created attachment 327921 [details] [review]
rtpsession: Add test for locking of the stats signal

Keeping the lock while emitting the stats signal introduces potential
deadlock in those situations when the signal callback wants the access
to rtpsession's properties which also requre the lock.
Comment 4 Sebastian Dröge (slomo) 2016-05-15 09:36:11 UTC
Review of attachment 327921 [details] [review]:

::: tests/check/elements/rtpsession.c
@@ +699,3 @@
+  /* then ask explicitly to send RTCP now */
+  g_signal_emit_by_name (internal_session, "send-rtcp-full", 0, &ret);
+  fail_unless (ret == TRUE);

This assertion fails
Comment 5 Mikhail Fludkov 2016-05-16 07:59:44 UTC
Created attachment 327959 [details] [review]
rtpsession: Add test for locking of the stats signal

The test should be fixed now. I've also refactored it bit.
Comment 6 Sebastian Dröge (slomo) 2016-05-20 06:26:29 UTC
Attachment 327959 [details] pushed as fa1c711 - rtpsession: Add test for locking of the stats signal