[Fdo-trac] [fdo-trac] #895: SQLite unit test deadlock

[FDO]

#895: SQLite unit test deadlock
-----------------------------+----------------------------
 Reporter:  jng              |        Owner:  traianstanev
     Type:  defect           |       Status:  closed
 Priority:  critical         |    Milestone:
Component:  SQLite Provider  |      Version:
 Severity:  1                |   Resolution:  fixed
 Keywords:                   |  External ID:
-----------------------------+----------------------------
Changes (by jng):

 * resolution:   => fixed
 * status:  new => closed

Comment:

 In [changeset:"8373" 8373]:
 {{{#!CommitTicketReference repository="" revision="8373"
 SQLite: Re-apply the metadata locking of r7133 in a form that also works
 on Linux

 r7133 ("#891 Sqlite Provider: Access Violation exception in method
 BuildMetaData()") fixed a
 Windows access violation caused by two threads building the provider's
 metadata cache at the
 same time by serializing SltMetadata::BuildMetadataInfo() in
 SltConnection::DescribeSchema()
 with m_csMd. It was reverted in r7150 because it made the Linux unit tests
 hang. The locking
 itself was fine; the platform underneath it was not.

 STL_CRITICAL_SECTION in SltProvider.h is the Win32 CRITICAL_SECTION on
 Windows, which is
 recursive - the owning thread may enter it again - but a plain
 pthread_mutex_init(m, NULL)
 mutex on POSIX, which is not. The metadata build nests:
 BuildMetadataInfo() ->
 SltMetadata::ProcessViewProperties() -> SltConnection::GetMetadata(),
 which takes m_csMd again
 for the geometry table of a view. On Windows that inner acquisition just
 bumps the recursion
 count; on Linux it waited forever on a lock only the waiting thread could
 release, which is
 exactly the stack in ticket 895 (SelectTest::TestViewSelects ->
 SltDescribeSchema::Execute ->
 SltMetadata::BuildMetadataInfo -> SltMetadata::ProcessViewProperties ->
 SltConnection::GetMetadata -> CriticalSectionHolder, blocked in
 pthread_mutex_lock).

 The POSIX branch of the abstraction now initializes its mutexes as
 PTHREAD_MUTEX_RECURSIVE via
 pthread_mutexattr_settype(), so the shim has the semantics the rest of the
 provider already
 assumes - CriticalSectionHolder and the query-cache code were written for
 a lock that can be
 re-entered - and on top of that r7133's two m_csMd acquisitions in
 DescribeSchema() are
 restored: around BuildMetadataInfo() in the requested-class-names branch,
 and before
 BuildMetadataInfo() in the full-schema path rather than after it.

 This also removes a latent deadlock that is in the 4.2 tree today,
 independent of r7133:
 GetMetadata() already holds m_csMd while calling BuildMetadataInfo(), so
 any view whose
 geometry table is itself a view was one m_bUseFdoMetadata away from the
 same hang.

 The three ForceLeaveCriticalSection() calls in the query-cache paths are
 left alone. They were
 added for the same non-recursive-mutex reason, so they are now redundant
 for correctness, but
 they also release the lock while sqlite3_reset()/sqlite3_prepare_v2() run,
 and changing that
 lock hold time is a separate decision.

 Verified on the baseline distro (ubuntu22, Debug): with only the
 DescribeSchema() locking
 applied the suite hangs - single-threaded process, no CPU burned, blocked
 in futex_do_wait -
 and with the recursive mutex as well it is green again: SelectTest alone
 passes (OK (33)) and
 the full suite passes (OK (89)) with no leaks, including the
 FdoMultiThreadTest query and
 insert tests that this locking is meant to protect.

 Fixes #891
 Fixes #895
 }}}
-- 
Ticket URL: <https://trac.osgeo.org/fdo/ticket/895#comment:1>
FDO <http://fdo.osgeo.org/>
Feature Data Objects