[libcamera-devel] [PATCH 00/12] libcamera: Hardening against thread race conditions

[Laurent Pinchart]

Hello,

Bug https://bugs.libcamera.org/show_bug.cgi?id=208 reports a race
condition that is ultimately due to incorrect usage of the libcamera
multi-threading infrastructure by the soft ISP code under development.
Instead of blaming the author of that code, I believe it shows we're not
doing well enough at communicating the threading requirements. This
patch series is an attempt to improve the situation.

libcamera has a threading model that is documented. In particular, the
documentation classifies functions as thread-safe, thread-bound and
reentrant. Patch 01/12 starts by fixing a minor issue in the
documentation of a thread-bound function that does not contain the
correct reference.

Patch 02/12 is a drive-by improvement that is, strictly speaking,
unrelated to this series, but was developed at the same time.

The next two patches fix a first thread-related issues with the
Object::deleteLater() function. Patch 03/12 extends a unit test to
display the issue, and patch 04/12 fixes it.

Patches 05/12 to 10/12 continue with fixing various kinds of incorrect
deletion of objects in unit tests. It turned out that all the race
conditions related to this patch series are in unit tests, libcamera
itself isn't (as far as I could see) affected. This is good news, even
if it means we haven't been careful enough when writing unit tests,
which calls for improvements in that area in the future.

Patches 11/12 and 12/12 finally add assertions to complain loudly about
incorrect deletion of Object instances (11/12) and incorrect calling
contexts of thread-bound functions (12/12).

Only the thread-bound members of Object subclasses have been hardened,
more work is needed to extend this to individual members of other
classes (currently only DeviceEnumerator::enumerate()), and to classes
that are marked as thread-bound at the class level (IPCUnixSocket and
V4L2VideoDevice).

Even if V4L2VideoDevice isn't addressed directly by this series, and the
bug report is related to usage of the V4L2VideoDevice class, I believe
the issue in the soft ISP code will be caught by the assertions added to
the EventNotifier class. Milan, would you be able to test this, and
confirm that libcamera now complains loudly ?

Laurent Pinchart (12):
  libcamera: object: Fix thread-bound reference in documentation
  libcamera: signal: Replace object.h inclusion with forward declatation
  test: object-delete: Test deferred delete just before thread stops
  libcamera: thread: Ensure deferred deletion of all objects before
    stopping
  test: event-thread: Destroy Object from correct thread context
  test: message: Remove incorrect slow receiver test
  test: message: Destroy Object from correct thread context
  test: signal-threads: Destroy Object from correct thread context
  test: timer-thread: Move timer start from wrong thread to separate
    test
  test: timer-thread: Destroy Object from correct thread context
  libcamera: object: Document and ensure Object deletion constraints
  libcamera: object: Add and use thread-bound assertion

 include/libcamera/base/object.h               |   2 +
 include/libcamera/base/signal.h               |   3 +-
 src/libcamera/base/bound_method.cpp           |   1 +
 src/libcamera/base/event_notifier.cpp         |   6 +
 src/libcamera/base/object.cpp                 |  50 +++++++-
 src/libcamera/base/signal.cpp                 |   1 +
 src/libcamera/base/thread.cpp                 |   7 ++
 src/libcamera/base/timer.cpp                  |  10 +-
 test/event-thread.cpp                         |  38 +++++--
 test/ipa/ipa_interface_test.cpp               |   1 +
 test/meson.build                              |   9 +-
 test/message.cpp                              |  54 +++------
 test/object-delete.cpp                        |  30 ++++-
 test/signal-threads.cpp                       |  24 ++--
 test/timer-fail.cpp                           | 107 ++++++++++++++++++
 test/timer-thread.cpp                         |  37 ++----
 .../module_ipa_proxy.h.tmpl                   |   1 +
 17 files changed, 276 insertions(+), 105 deletions(-)
 create mode 100644 test/timer-fail.cpp


base-commit: 0d99f2de13faccf199e6c9e806702cb21d0b6c24
-- 
Regards,

Laurent Pinchart