[Gstreamer-bugs] Re: [Bug 108263] Changed - Possible race condition in gstqueue.c

[Martin Janzen]

bugzilla-daemon at widget.gnome.org wrote:
 > http://bugzilla.gnome.org/show_bug.cgi?id=108263
 >
 > Changed by in7y118 at public.uni-hamburg.de.
 >
 > --- shadow/108263	Wed Mar 12 23:21:20 2003
 > +++ shadow/108263.tmp.4445	Fri Mar 14 12:22:00 2003
 > @@ -38,6 +38,26 @@
 >  the call to g_mutex_unlock(); see the attached patch.
 >
 >  ------- Additional Comments From janzen at pixelmetrix.com  2003-03-12 
23:21 -------
 >  Created an attachment (id=14981)
 >  Patch for unlikely race condition in gstqueue.c
 >
 > +
 > +------- Additional Comments From in7y118 at public.uni-hamburg.de 
2003-03-14 12:21 -------
 > +I believe the current behaviour is correct.
 > +
 > +Imagine the following scenario:
 > +- The queue is full.
 > +- A new buffer arrives. The chain function returns via
 > +gst_scheduler_interrupt to let the scheduler go on.
 > +- A flush event arrives on the other side of the queue, the queue is
 > +emptied.
 > +- The scheduler reschedules the queue, the chain function goes on
 > +executing. The queue has been flushed, but we're still holding a
 > +buffer.
 > +In this case the buffer should be discarded and not put into the
 > +queue, which is exactly what is done.

OK, thanks for reviewing this!  Let's see if I understand:

388:    while (queue->level_buffers == queue->size_buffers) {
389:      /* if there's a pending state change for this queue or its 
manager, switch */
390:      /* back to iterator so bottom half of state change executes */
391:      if (queue->interrupt) {
392:        GST_DEBUG_ELEMENT (GST_CAT_DATAFLOW, queue, "interrupted!!");
393:        g_mutex_unlock (queue->qlock);
394:	if (gst_scheduler_interrupt (gst_pad_get_scheduler 
(queue->sinkpad), GST_ELEMENT (queue)))
395:          return;
396:	/* if we got here bacause we were unlocked after a flush, we don't need
397:	 * to add the buffer to the queue again */
398:	if (queue->flush) {
399:          GST_DEBUG_ELEMENT (GST_CAT_DATAFLOW, queue, "not adding 
pending buffer after flush");
400:	  return;
401:	}
402:        GST_DEBUG_ELEMENT (GST_CAT_DATAFLOW, queue, "adding pending 
buffer after interrupt");
403:	goto restart;
404:      }

You're saying that if the queue->interrupt flag is set, the mutex
unlocked, and gst_scheduler_interrupt() returns FALSE, then at some
point between the unlock in 393 and the test in 398 the queue could
have been flushed; in which case we want to return immediately based
on the new state of queue->flush, not on its state before the unlock.

If that's the case, then fair enough; my patch should not be applied,
and #108263 changed to RESOLVED/NOTABUG.  (Also, it means I'll have to
remove it from the enhancement I posted as #108268.  Moral of the
story: Keep these separate!!)  Sorry for the false alarm.


 > +As a sidenote:
 > +We're missing a lot of gst_buffer_unref's when returning from
 > +erroneous behaviour.

Yes, and line 400 looks like one instance of this.  But maybe I'll
let someone more familiar with schedulers, interrupts, etc. propose
that patch! :)

Cheers...

MJ