mythbackend deadlock when editing schedule metadata on frontend

[Peter Bennett]

This has happened a few times, but only since V30.

After performing a search for artwork in the schedule editor, the backend sometimes gets into a state where the frontends lose the connection and no frontend can get connected again.

[Peter Bennett]

Full backtrace

[Peter Bennett]

Version

[Peter Bennett]

This happened again, this time with no schedule or unusual actions on the frontend, just connecting the frontend and navigating to the recordings list.

[Peter Bennett]

This may be related to the report in the forum at ​https://forum.mythtv.org/viewtopic.php?f=36&t=3545

[Peter Bennett]

See the attached file thread_analysis.txt

Thread 34 and Thread 6 are both doing this

MainServer::connectionClosed calls sockListLock.lockForWrite
MainServer::connectionClosed calls MainServer::UpdateSystemdStatus
MainServer::UpdateSystemdStatus calls m_sched->GetAllPending(recordings)
Scheduler::GetAllPending calls QMutexLocker lockit(&schedLock)

This causes

1. lock on sockListLock
2. lock on schedLock

Thread 22 is doing this

Scheduler::run calls QMutexLocker lockit(&schedLock); (l2070)
Scheduler::run calls HandleIdleShutdown; (l2234)
Scheduler::HandleIdleShutdown calls MainServer::isClientConnected :8358
MainServer::isClientConnected calls sockListLock.lockForRead

This causes

1. lock on schedLock
2. lock on sockListLock

* DEADLOCK *

If you are setting two or more locks, you need to always set them in the same order in every thread, otherwise deadlocks can happen.

There are many places where mainserver calls scheduler and fewer than 10 places where scheduler calls mainserver. All are potential deadlock scenarios. Perhaps whenever scheduler calls mainserver it should first unlock its mutex and lock it again upon return.

ANY COMMENTS FROM DEVELOPERS?

[Peter Bennett]

Thread Analysis

[Peter Bennett]

scheduler.cpp already has 9 places where calls to other routines are surrounded by schedLock.unlock() and schedLock.lock(). It looks like I only need to add those to the two places in scheduler.cpp where m_mainServer->isClientConnected() is called.

[Peter Bennett]

Regarding m_recListChanged - this can be used to check if the list changed. If the list changes during the call to m_mainServer->isClientConnected, that will not be a problem. In HandleIdleShutdown it does look through the list, and the list cannot change while it is looking through the list since it is locked at that time. I prefer to make as few changes as possible to reduce risk, so I plan to only unlock and relock at the two places m_mainServer->isClientConnected is called inside HandleIdleShutdown.

[gigem]

Replying to Peter Bennett:

Regarding m_recListChanged - this can be used to check if the list changed. If the list changes during the call to m_mainServer->isClientConnected, that will not be a problem. In HandleIdleShutdown it does look through the list, and the list cannot change while it is looking through the list since it is locked at that time. I prefer to make as few changes as possible to reduce risk, so I plan to only unlock and relock at the two places m_mainServer->isClientConnected is called inside HandleIdleShutdown.

Please do not do this. When m_recListChanged is true, stop and return. Always. It's not just that iterators might have been invalidated, but other assumptions like list ordering might have been invalidated too. You might be able to convince yourself that in this case at this time, it won't hurt anything. That might not always be the case, though, as an unrelated, future change might invalidate it. To avoid that and keep things simple, just stop and return.

[Peter Bennett]

Updated V30 patch

[Peter Bennett]

Updated V31 (master) patch

[Peter Bennett <pbennett@…>]

In 3e9e8353a/mythtv:

Scheduler: Fix deadlock in mythbackend

Fixes deadlock caused by conflicting Mutex locks in HandleIdleShutdown?

Fixes #13479

[Peter Bennett <pbennett@…>]

In 47814b6d6/mythtv:

Scheduler: Fix deadlock in mythbackend

Fixes deadlock caused by conflicting Mutex locks in HandleIdleShutdown?

Fixes #13479