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 706831 - composition: Release objects lock while forwarding an event
composition: Release objects lock while forwarding an event
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gnonlin
unspecified
Other All
: Normal major
: 1.2.0
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2013-08-26 21:09 UTC by Thibault Saunier
Modified: 2014-01-06 10:49 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
composition: Release objects lock while forwarding an event (1.99 KB, patch)
2013-08-26 21:09 UTC, Thibault Saunier
committed Details | Review
composition: Never take the OBJECTS_LOCK when sending events (2.69 KB, patch)
2013-09-06 23:46 UTC, Thibault Saunier
none Details | Review
composition: Never take the OBJECTS_LOCK when sending events (2.69 KB, patch)
2013-09-06 23:47 UTC, Thibault Saunier
accepted-commit_now Details | Review

Description Thibault Saunier 2013-08-26 21:09:35 UTC
There is no reason to keep it and in some rare cases it creates
deadlocks as followed:
  t1:
     → Composition receives a flushing seek, it takes the OBJECTS_LOCK
        and fowards the flushing seek event upstream
     → adder receives the seek and set its collectpad to flushing
        This implies tacking STREAM_LOCK (collectpad)

  t2:
    → Collectpad has buffers ready, and has the STREAM_LOCK
    (collectpad) and is EOS, so it sends it downstream
    → The composition receives EOS, and needs to check if it is
    the actual EOS or not, thus need to take the OBJECTS_LOCK

This create a deadlock, and in the first stage, we did not need the
OBJECTS_LOCK to forward downstream the flushing seek, so do not take
it.
Comment 1 Thibault Saunier 2013-08-26 21:09:38 UTC
Created attachment 253177 [details] [review]
composition: Release objects lock while forwarding an event

There is no reason to keep it and in some rare cases it creates
deadlocks as followed:
  t1:
     → Composition receives a flushing seek, it takes the OBJECTS_LOCK
        and fowards the flushing seek event upstream
     → adder receives the seek and set its collectpad to flushing
        This implies tacking STREAM_LOCK (collectpad)

  t2:
    → Collectpad has buffers ready, and has the STREAM_LOCK
    (collectpad) and is EOS, so it sends it downstream
    → The composition receives EOS, and needs to check if it is
    the actual EOS or not, thus need to take the OBJECTS_LOCK

This create a deadlock, and in the first stage, we did not need the
OBJECTS_LOCK to forward downstream the flushing seek, so do not take
it.
Comment 2 Sebastian Dröge (slomo) 2013-08-27 08:12:35 UTC
Review of attachment 253177 [details] [review]:

The object lock should *never* be hold while sending a buffer, event, query or message.
Comment 3 Thibault Saunier 2013-08-27 22:49:39 UTC
Attachment 253177 [details] pushed as 8f3bb08 - composition: Release objects lock while forwarding an event
Comment 4 Thibault Saunier 2013-09-06 23:46:10 UTC
Created attachment 254322 [details] [review]
composition: Never take the OBJECTS_LOCK when sending events

We should never have the OBJECTS_LOCK when sending or receiving
events. This lock is usefull only when we deal with children of
composition, so it should be used exclusivly for that purpose

It avoiding deadlocks
Comment 5 Thibault Saunier 2013-09-06 23:47:16 UTC
Created attachment 254323 [details] [review]
composition: Never take the OBJECTS_LOCK when sending events

We should never have the OBJECTS_LOCK when sending or receiving
events. This lock is usefull only when we deal with children of
composition, so it should be used exclusivly for that purpose

Avoiding deadlocks
Comment 6 Sebastian Dröge (slomo) 2013-09-09 11:02:28 UTC
Review of attachment 254323 [details] [review]:

Looks good but please make sure again that you don't introduce race conditions by releasing the lock for a short time, i.e. does code assume below that nothing has changed compared to before sending the event? If code assumes that, rewrite it or introduce a new lock.
Comment 7 Sebastian Dröge (slomo) 2014-01-06 09:23:35 UTC
Thibault?
Comment 8 Thibault Saunier 2014-01-06 10:49:33 UTC
commit 8f3bb083f8e59d9c541c4f80ed40a1b9c780e7dd
Author: Thibault Saunier <thibault.saunier@collabora.com>
Date:   Mon Aug 26 16:26:31 2013 -0400

    composition: Release objects lock while forwarding an event
    
    There is no reason to keep it and in some rare cases it creates
    deadlocks as followed:
      t1:
         → Composition receives a flushing seek, it takes the OBJECTS_LOCK
            and fowards the flushing seek event upstream
         → adder receives the seek and set its collectpad to flushing
            This implies tacking STREAM_LOCK (collectpad)
    
      t2:
        → Collectpad has buffers ready, and has the STREAM_LOCK
        (collectpad) and is EOS, so it sends it downstream
        → The composition receives EOS, and needs to check if it is
        the actual EOS or not, thus need to take the OBJECTS_LOCK
    
    This create a deadlock, and in the first stage, we did not need the
    OBJECTS_LOCK to forward downstream the flushing seek, so do not take
    it.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=706831