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 711138 - iterator: Don't hold mutex while calling filter functions of recursive filter iterators
iterator: Don't hold mutex while calling filter functions of recursive filter...
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gstreamer (core)
1.2.0
Other All
: Normal normal
: 1.3.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2013-10-30 11:50 UTC by Stewart Brodie
Modified: 2014-01-18 13:51 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Larger version of the example code (837 bytes, text/plain)
2013-10-30 11:50 UTC, Stewart Brodie
  Details
Patch to preserve filter_next()'s access to the mutex (1.79 KB, patch)
2013-10-30 11:53 UTC, Stewart Brodie
committed Details | Review

Description Stewart Brodie 2013-10-30 11:50:06 UTC
Created attachment 258558 [details]
Larger version of the example code

The general problem is that the filter_next function in gstiterator.c carefully releases the mutex whilst it calls the filter callback function - unless it has been wrapped by another filter, in which case it doesn't release the mutex, leading to possible deadlocks in the filter callback function.

The specific problem: I am trying to iterate through the caps structure of linked pads looking for pads with caps that match a particular type.  The way I tried to do this was to chain several filters (from gst_iterator_filter) in series to allow the application to call gst_iterator_foreach() with the final iterator.  However, deadlock occurs when my caps checking filter calls gst_pad_query_caps(), as that function tries to claim the owning GstElement's mutex that the GstIteratorFilter has still got claimed.

{
  GstIterator *it;
  it = gst_element_iterate_src_pads( element );
  it = gst_iterator_filter( it, filter_test_caps, NULL );
  it = gst_iterator_filter( it, filter_test_linked, NULL );

  while ( gst_iterator_foreach( it, ... ) != GST_ITERATOR_RESYNC ) { ... }
}

gint filter_test_caps( gconstpointer pad, gconstpointer unused )
{
    /* The following call to gst_pad_query_caps hangs */
    GstCaps *caps = gst_pad_query_caps(GST_PAD(g_value_get_object(pad)),NULL);
    ...
}

Obviously, the much-simplified example code excerpted here (copy attached) could be easily refactored to use a single filter.  However, in the code's real environment, that isn't really feasible.


Explanation: when gst_iterator_filter() wraps another iterator, it steals the mutex from the wrapped iterator for itself so when filter_next of the wrapped iterator tries to release the mutex, it can't, as it's not there any more.


A potential solution: wrapped filters must also release the mutex whilst calling their filter functions.  I attach a small patch that would do this by retaining a private copy of the original mutex in the private GstIteratorFilter data structure and having filter_next() look at that mutex instead of the main iterator mutex.  I think this is all fine and safe (and it fixes the problem I've got) - but would appreciate review by others.  The patch is made against 1.0.7, but is the same for 1.2.0 (gstiterator.c hasn't changed between the two versions)


Another way of solving the problem might be for all GstIterators to store a list of filter functions that are all called in order - in other words, instead of having multiple iterator objects, have one iterator object with a GList of filter functions that it can run through in sequence (using an iterator!)  I've not tried that, but it seems a sensible enough idea.
Comment 1 Stewart Brodie 2013-10-30 11:53:00 UTC
Created attachment 258559 [details] [review]
Patch to preserve filter_next()'s access to the mutex
Comment 2 Sebastian Dröge (slomo) 2014-01-18 13:51:23 UTC
commit bac1202cf7c8a3aeef14a1328f0651393f52175b
Author: Sebastian Dröge <sebastian@centricular.com>
Date:   Sat Jan 18 14:48:35 2014 +0100

    iterator: Properly copy mutexes around when creating a copy of a filter iterator

commit af84535569342e2e4b35e700e60463335e676c72
Author: Sebastian Dröge <sebastian@centricular.com>
Date:   Sat Jan 18 14:34:45 2014 +0100

    iterator: Add unit tests for filtering, recursive filtering and locking
    
    https://bugzilla.gnome.org/show_bug.cgi?id=711138

commit f77d79f2f8eae2e10b664ef83e7ea97a4b9e762b
Author: Stewart Brodie <stewart@eh.org>
Date:   Sat Jan 18 14:43:20 2014 +0100

    iterator: Preserve the master lock when creating recursive iterator filters with the same lock
    
    This way we make sure that a) the lock is always taken when checking
    the cookie and calling the iterator's next functions and b) it is
    not taken while calling any of the iterator filter functions.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=711138