| Message ID | 20240121035948.4226-4-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes: > The Object::deleterLater() function is expected to not race with ^^^ deleteLater > stopping the thread the object is bound to. Add a test for this. > > The test currently fails, demonstrating a bug in libcamera. Shouldn't it then be marked as an expected failure? > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > test/object-delete.cpp | 30 +++++++++++++++++++++++++----- > 1 file changed, 25 insertions(+), 5 deletions(-) > > diff --git a/test/object-delete.cpp b/test/object-delete.cpp > index eabefe935974..80b7dc41cd37 100644 > --- a/test/object-delete.cpp > +++ b/test/object-delete.cpp > @@ -33,10 +33,10 @@ public: > unsigned int *deleteCount_; > }; > > -class NewThread : public Thread > +class DeleterThread : public Thread > { > public: > - NewThread(Object *obj) > + DeleterThread(Object *obj) > : object_(obj) > { > } > @@ -63,9 +63,9 @@ protected: > unsigned int count = 0; > TestObject *obj = new TestObject(&count); > > - NewThread thread(obj); > - thread.start(); > - thread.wait(); > + DeleterThread delThread(obj); > + delThread.start(); > + delThread.wait(); > > Thread::current()->dispatchMessages(Message::Type::DeferredDelete); > > @@ -89,6 +89,26 @@ protected: > return TestFail; > } > > + /* > + * Test that deleteLater() works properly when called just > + * before the object's thread exits. > + */ > + Thread boundThread; > + boundThread.start(); > + > + count = 0; > + obj = new TestObject(&count); > + obj->moveToThread(&boundThread); > + > + obj->deleteLater(); > + boundThread.exit(); > + boundThread.wait(); > + > + if (count != 1) { > + cout << "Object deletion right before thread exit failed (" << count << ")" << endl; > + return TestFail; > + } > + > return TestPass; > } > };
On Mon, Jan 22, 2024 at 08:22:10PM +0100, Milan Zamazal wrote: > Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes: > > > The Object::deleterLater() function is expected to not race with > ^^^ > deleteLater > > > stopping the thread the object is bound to. Add a test for this. > > > > The test currently fails, demonstrating a bug in libcamera. > > Shouldn't it then be marked as an expected failure? I don't think so. should_fail in meson indicates that the test executable returning an error (including terminating due to an uncaught signal, such as SIGSEGV) indicates success. In this case, the test should pass, but a there's bug in libcamera that makes it fail. Once the bug gets fixed, the test will then pass. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > test/object-delete.cpp | 30 +++++++++++++++++++++++++----- > > 1 file changed, 25 insertions(+), 5 deletions(-) > > > > diff --git a/test/object-delete.cpp b/test/object-delete.cpp > > index eabefe935974..80b7dc41cd37 100644 > > --- a/test/object-delete.cpp > > +++ b/test/object-delete.cpp > > @@ -33,10 +33,10 @@ public: > > unsigned int *deleteCount_; > > }; > > > > -class NewThread : public Thread > > +class DeleterThread : public Thread > > { > > public: > > - NewThread(Object *obj) > > + DeleterThread(Object *obj) > > : object_(obj) > > { > > } > > @@ -63,9 +63,9 @@ protected: > > unsigned int count = 0; > > TestObject *obj = new TestObject(&count); > > > > - NewThread thread(obj); > > - thread.start(); > > - thread.wait(); > > + DeleterThread delThread(obj); > > + delThread.start(); > > + delThread.wait(); > > > > Thread::current()->dispatchMessages(Message::Type::DeferredDelete); > > > > @@ -89,6 +89,26 @@ protected: > > return TestFail; > > } > > > > + /* > > + * Test that deleteLater() works properly when called just > > + * before the object's thread exits. > > + */ > > + Thread boundThread; > > + boundThread.start(); > > + > > + count = 0; > > + obj = new TestObject(&count); > > + obj->moveToThread(&boundThread); > > + > > + obj->deleteLater(); > > + boundThread.exit(); > > + boundThread.wait(); > > + > > + if (count != 1) { > > + cout << "Object deletion right before thread exit failed (" << count << ")" << endl; > > + return TestFail; > > + } > > + > > return TestPass; > > } > > };
Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes: > On Mon, Jan 22, 2024 at 08:22:10PM +0100, Milan Zamazal wrote: >> Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes: >> > >> > The Object::deleterLater() function is expected to not race with >> ^^^ >> deleteLater >> >> > stopping the thread the object is bound to. Add a test for this. >> > >> > The test currently fails, demonstrating a bug in libcamera. >> >> Shouldn't it then be marked as an expected failure? > > I don't think so. should_fail in meson indicates that the test > executable returning an error (including terminating due to an uncaught > signal, such as SIGSEGV) indicates success. In this case, the test > should pass, but a there's bug in libcamera that makes it fail. Once the > bug gets fixed, the test will then pass. My point was whether it is OK to have a commit with a failing test, whether it can disturb anything. Right, marking it as an expected failure would be wrong, but maybe disabling it's automatic run and enabling it in the fix commit would be a better idea? >> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Anyway, I don't have anything else to add, so: Reviewed-by: Milan Zamazal <mzamazal@redhat.com> >> > --- >> > test/object-delete.cpp | 30 +++++++++++++++++++++++++----- >> > 1 file changed, 25 insertions(+), 5 deletions(-) >> > >> > diff --git a/test/object-delete.cpp b/test/object-delete.cpp >> > index eabefe935974..80b7dc41cd37 100644 >> > --- a/test/object-delete.cpp >> > +++ b/test/object-delete.cpp >> > @@ -33,10 +33,10 @@ public: >> > unsigned int *deleteCount_; >> > }; >> > >> > -class NewThread : public Thread >> > +class DeleterThread : public Thread >> > { >> > public: >> > - NewThread(Object *obj) >> > + DeleterThread(Object *obj) >> > : object_(obj) >> > { >> > } >> > @@ -63,9 +63,9 @@ protected: >> > unsigned int count = 0; >> > TestObject *obj = new TestObject(&count); >> > >> > - NewThread thread(obj); >> > - thread.start(); >> > - thread.wait(); >> > + DeleterThread delThread(obj); >> > + delThread.start(); >> > + delThread.wait(); >> > >> > Thread::current()->dispatchMessages(Message::Type::DeferredDelete); >> > >> > @@ -89,6 +89,26 @@ protected: >> > return TestFail; >> > } >> > >> > + /* >> > + * Test that deleteLater() works properly when called just >> > + * before the object's thread exits. >> > + */ >> > + Thread boundThread; >> > + boundThread.start(); >> > + >> > + count = 0; >> > + obj = new TestObject(&count); >> > + obj->moveToThread(&boundThread); >> > + >> > + obj->deleteLater(); >> > + boundThread.exit(); >> > + boundThread.wait(); >> > + >> > + if (count != 1) { >> > + cout << "Object deletion right before thread exit failed (" << count << ")" << endl; >> > + return TestFail; >> > + } >> > + >> > return TestPass; >> > } >> > };
diff --git a/test/object-delete.cpp b/test/object-delete.cpp index eabefe935974..80b7dc41cd37 100644 --- a/test/object-delete.cpp +++ b/test/object-delete.cpp @@ -33,10 +33,10 @@ public: unsigned int *deleteCount_; }; -class NewThread : public Thread +class DeleterThread : public Thread { public: - NewThread(Object *obj) + DeleterThread(Object *obj) : object_(obj) { } @@ -63,9 +63,9 @@ protected: unsigned int count = 0; TestObject *obj = new TestObject(&count); - NewThread thread(obj); - thread.start(); - thread.wait(); + DeleterThread delThread(obj); + delThread.start(); + delThread.wait(); Thread::current()->dispatchMessages(Message::Type::DeferredDelete); @@ -89,6 +89,26 @@ protected: return TestFail; } + /* + * Test that deleteLater() works properly when called just + * before the object's thread exits. + */ + Thread boundThread; + boundThread.start(); + + count = 0; + obj = new TestObject(&count); + obj->moveToThread(&boundThread); + + obj->deleteLater(); + boundThread.exit(); + boundThread.wait(); + + if (count != 1) { + cout << "Object deletion right before thread exit failed (" << count << ")" << endl; + return TestFail; + } + return TestPass; } };
The Object::deleterLater() function is expected to not race with stopping the thread the object is bound to. Add a test for this. The test currently fails, demonstrating a bug in libcamera. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- test/object-delete.cpp | 30 +++++++++++++++++++++++++----- 1 file changed, 25 insertions(+), 5 deletions(-)