Bug 737794 – multiqueue: deadlock if queue overruns with serialized events

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 737794 - multiqueue: deadlock if queue overruns with serialized events


Summary:	multiqueue: deadlock if queue overruns with serialized events


Status:	RESOLVED FIXED

Product:	GStreamer
Classification:	Platform
Component:	gstreamer (core)
Version:	git master
Hardware:	Other Linux

Importance:	Normal normal
Target Milestone:	1.4.4
Assigned To:	GStreamer Maintainers
QA Contact:	GStreamer Maintainers

URL:
Whiteboard:

Depends on:
Blocks:

Reported:	2014-10-02 17:22 UTC by Aleix Conchillo Flaqué
Modified:	2014-10-14 07:43 UTC

See Also:
GNOME target:	---
GNOME version:	---

Attachments
release lock before pushing the serialized event (1.30 KB, patch) 2014-10-02 17:26 UTC, Aleix Conchillo Flaqué	needs-work	Details \| Review
release lock before pushing a serialized query (2.02 KB, patch) 2014-10-08 16:41 UTC, Aleix Conchillo Flaqué	none	Details \| Review
release lock before pushing a serialized query (2.40 KB, patch) 2014-10-09 04:43 UTC, Aleix Conchillo Flaqué	committed	Details \| Review

Description Aleix Conchillo Flaqué 2014-10-02 17:22:50 UTC

If a serialzed event goes into a queue but the queue overruns we hit a deadlock on the multiqueue lock. See Thread 21:

+ Trace 234165

Thread 21 (Thread 0x7f05d4ff9700 (LWP 59217))

#0 __lll_lock_wait
at ../nptl/sysdeps/unix/sysv/linux/x86_64/lowlevellock.S line 132
#1 _L_lock_1006
from /lib/x86_64-linux-gnu/libpthread.so.0
#2 __pthread_mutex_lock
at pthread_mutex_lock.c line 101
#3 g_mutex_lock
at gthread-posix.c line 216
#4 single_queue_overrun_cb
at gstmultiqueue.c line 2058
#5 gst_data_queue_push
at gstdataqueue.c line 511
#6 gst_multi_queue_sink_query
at gstmultiqueue.c line 1822
#7 gst_pad_query
at gstpad.c line 3584
#8 gst_pad_peer_query
at gstpad.c line 3715
#9 query_forward_func
at gstpad.c line 3040
#10 gst_pad_forward
at gstpad.c line 2796
#11 gst_pad_query_default
at gstpad.c line 3104
#12 gst_pad_query
at gstpad.c line 3584
#13 gst_pad_peer_query
at gstpad.c line 3715
#14 gst_queue_push_one
at gstqueue.c line 1214
#15 gst_queue_loop
at gstqueue.c line 1277
#16 gst_task_func
at gsttask.c line 317
#17 g_thread_pool_thread_proxy
at gthreadpool.c line 307
#18 g_thread_proxy
at gthread.c line 764
#19 start_thread
at pthread_create.c line 308
#20 clone
at ../sysdeps/unix/sysv/linux/x86_64/clone.S line 112
#21 ??

Thread 19 (Thread 0x7f05a3fff700 (LWP 59221))

#0 __lll_lock_wait
at ../nptl/sysdeps/unix/sysv/linux/x86_64/lowlevellock.S line 132
#1 _L_lock_1006
from /lib/x86_64-linux-gnu/libpthread.so.0
#2 __pthread_mutex_lock
at pthread_mutex_lock.c line 101
#3 g_mutex_lock
at gthread-posix.c line 216
#4 gst_multi_queue_loop
at gstmultiqueue.c line 1460
#5 gst_task_func
at gsttask.c line 317
#6 g_thread_pool_thread_proxy
at gthreadpool.c line 307
#7 g_thread_proxy
at gthread.c line 764
#8 start_thread
at pthread_create.c line 308
#9 clone
at ../sysdeps/unix/sysv/linux/x86_64/clone.S line 112
#10 ??

Thread 55 (Thread 0x7f05f6bbd700 (LWP 55195))

#0 __lll_lock_wait
at ../nptl/sysdeps/unix/sysv/linux/x86_64/lowlevellock.S line 132
#1 _L_lock_1006
from /lib/x86_64-linux-gnu/libpthread.so.0
#2 __pthread_mutex_lock
at pthread_mutex_lock.c line 101
#3 g_mutex_lock
at gthread-posix.c line 216
#4 gst_multi_queue_change_state
at gstmultiqueue.c line 799
#5 gst_element_change_state
at gstelement.c line 2602
#6 gst_element_set_state_func
at gstelement.c line 2558
#7 gst_bin_element_set_state
at gstbin.c line 2328
#8 gst_bin_change_state_func
at gstbin.c line 2665
#9 gst_element_change_state
at gstelement.c line 2602
#10 gst_element_set_state_func
at gstelement.c line 2558
#11 gst_bin_element_set_state
at gstbin.c line 2328
#12 gst_bin_change_state_func
at gstbin.c line 2665
#13 gst_element_change_state
at gstelement.c line 2602
#14 gst_element_set_state_func
at gstelement.c line 2558
#15 gst_bin_element_set_state
at gstbin.c line 2328
#16 gst_bin_change_state_func
at gstbin.c line 2665
#17 gst_element_change_state
at gstelement.c line 2602
#18 gst_element_change_state
at gstelement.c line 2646
#19 gst_element_set_state_func
at gstelement.c line 2558
#20 finish_unprepare
at rtsp-media.c line 2403
#21 gst_rtsp_media_unprepare
at rtsp-media.c line 2523
#22 gst_rtsp_media_prepare
at rtsp-media.c line 2385
#23 find_media
at rtsp-client.c line 619
#24 handle_describe_request
at rtsp-client.c line 2071
#25 handle_request
at rtsp-client.c line 2407
#26 gst_rtsp_client_handle_message
at rtsp-client.c line 2974
#27 gst_rtsp_source_dispatch_read
at gstrtspconnection.c line 3241
#28 g_main_dispatch
at gmain.c line 3064
#29 g_main_context_dispatch
at gmain.c line 3663
#30 g_main_context_iterate
at gmain.c line 3734
#31 g_main_context_iterate
at gmain.c line 3671
#32 g_main_loop_run
at gmain.c line 3928
#33 do_loop
at rtsp-thread-pool.c line 329
#34 g_thread_pool_thread_proxy
at gthreadpool.c line 307
#35 g_thread_proxy
at gthread.c line 764
#36 start_thread
at pthread_create.c line 308
#37 clone
at ../sysdeps/unix/sysv/linux/x86_64/clone.S line 112
#38 ??

Comment 1 Aleix Conchillo Flaqué 2014-10-02 17:26:53 UTC

Created attachment 287613 [details] [review]
release lock before pushing the serialized event

Comment 2 Aleix Conchillo Flaqué 2014-10-02 19:15:50 UTC

(In reply to comment #1)
> Created an attachment (id=287613) [details] [review]
> release lock before pushing the serialized event

The deadlock is there, but I'm not sure if this patch is correct. I'm observing a different behavior in my application.

Comment 3 Sebastian Dröge (slomo) 2014-10-06 07:00:43 UTC

Review of attachment 287613 [details] [review]:

::: plugins/elements/gstmultiqueue.c
@@ +1867,1 @@
           g_cond_wait (&sq->query_handled, &mq->qlock);

This causes a race condition. The query (not event!) can already be taken out of the queue again before you lock again and wait for the condition variable, leading to another deadlock caused by waiting forever for the condition variable to be signalled.

Why does the dataqueue block in your case? There's a check above that will only consider putting it in the dataqueue in certain conditions. Which is true in your case? !use_buffering? Why is the queue never emptying in your case, which would lead to the deadlock solving itself?

Comment 4 Sebastian Dröge (slomo) 2014-10-06 07:02:31 UTC

Oh I just noticed... the deadlock comes from the overrun callback which takes the same mutex. I think we need to unlock the mutex in any case then, but need to make sure that the above mentioned new deadlock does not happen.

For this you could remember if and which query was answered last, and check that before waiting for the condition variable.

Comment 5 Aleix Conchillo Flaqué 2014-10-08 16:41:47 UTC

Created attachment 288057 [details] [review]
release lock before pushing a serialized query

Let's see this one. It's working so far. I just save the last handled query and compare that before waiting on the condition variable.

Comment 6 Aleix Conchillo Flaqué 2014-10-09 04:43:08 UTC

Created attachment 288090 [details] [review]
 release lock before pushing a serialized query

Same patch as before with comments.

Comment 7 Sebastian Dröge (slomo) 2014-10-09 08:50:04 UTC

commit 41fa9ad9f293c2079b0a3bf6ccee08ec39753d91
Author: Aleix Conchillo Flaqué <aleix@oblong.com>
Date:   Wed Oct 8 09:37:41 2014 -0700

    multiqueue: don't lock multiqueue when pushing serialized queries
    
    If we are pushing a serialized query into a queue and the queue is
    filled, we will end in a deadlock. We need to release the lock before
    pushing and acquire it again afterward.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=737794