[Bug 750397] CRITICAL: Race condition in GstBus

[GStreamer (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=750397

--- Comment #59 from Matt Gruenke <mgruenke at tycoint.com> ---
(In reply to Sebastian Dröge (slomo) from comment #56)
> Review of attachment 330504 [details] [review]:
> 
> Looks good to me except for minor things

> @@ +206,3 @@
> +  if (set->control_pending > 0) {
> +    /* release, only if this was the last pending. */
> +    if (set->control_pending-- == 1) {
> 
> --set->control_pending == 0 seems more clear (in symmetry with set->control_pending++ == 0 above)

I was going for a 1:1 substitution of the atomic operations, but I can change
it.


> @@ +211,3 @@
> +    }
> +    else
> +      result = TRUE;
> 
> Curly braces here while we're at it :)

I was trying to be consistent with what I saw elsewhere in the file.  Will fix.


> @@ +230,3 @@
> +    GST_LOG ("%p: releasing %d", set, old);
> +    if (!RELEASE_EVENT (set))
> +      GST_WARNING ("%p: failed to release event", set);
> 
> Before this was a loop to call RELEASE_EVENT() until it succeeded

Here's my only real objection.  There's no reason to expect it'll fail, unless
someone passes in a bad GstPoll.  And if it does fail, there's no reason to
think that looping will result in success.  So, I'd rather try once and warn,
than introduce a hang.

If you prefer, I can emit a critical warning.

-- 
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.