| Message ID | 20221003235843.19175-1-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent, On 10/4/22 5:28 AM, Laurent Pinchart via libcamera-devel wrote: > If a thread ends abnormally (that is, without retuning normally from its > run() function, for instance with a direct call to pthread_cancel()), > thread cleanup should still be performed. Add a test to ensure this. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > test/threads.cpp | 44 ++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 44 insertions(+) > > diff --git a/test/threads.cpp b/test/threads.cpp > index d83b5833998f..7daa76edb00e 100644 > --- a/test/threads.cpp > +++ b/test/threads.cpp > @@ -8,7 +8,9 @@ > #include <chrono> > #include <iostream> > #include <memory> > +#include <pthread.h> > #include <thread> > +#include <time.h> > > #include <libcamera/base/thread.h> > > @@ -35,6 +37,32 @@ private: > chrono::steady_clock::duration duration_; > }; > > +class CancelThread : public Thread > +{ > +public: > + CancelThread(bool &cancelled) > + : cancelled_(cancelled) > + { > + } > + > +protected: > + void run() > + { > + cancelled_ = true; > + > + /* Cancel the thread can call a guaranteed cancellation point. */ > + pthread_cancel(pthread_self()); > + > + struct timespec req{ 0, 100*000*000 }; > + nanosleep(&req, nullptr); Took a bit of reading to determine that nanosleep() is a cancellation point :) Maybe it deserves a comment as well, upto you. Reviewed-by: Umang Jain <umang.jain@ideasonboard.com> > + > + cancelled_ = false; > + } > + > +private: > + bool &cancelled_; > +}; > + > class ThreadTest : public Test > { > protected: > @@ -118,6 +146,22 @@ protected: > return TestFail; > } > > + /* Test thread cleanup upon abnormal termination. */ > + bool cancelled = false; > + bool finished = false; > + > + thread = std::make_unique<CancelThread>(cancelled); > + thread->finished.connect(this, [&finished]() { finished = true; }); > + > + thread->start(); > + thread->exit(0); > + thread->wait(chrono::milliseconds(1000)); > + > + if (!cancelled || !finished) { > + cout << "Cleanup failed upon abnormal termination" << endl; > + return TestFail; > + } > + > return TestPass; > } >
On Tue, Oct 04, 2022 at 07:35:42PM +0530, Umang Jain wrote: > On 10/4/22 5:28 AM, Laurent Pinchart via libcamera-devel wrote: > > If a thread ends abnormally (that is, without retuning normally from its > > run() function, for instance with a direct call to pthread_cancel()), > > thread cleanup should still be performed. Add a test to ensure this. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > test/threads.cpp | 44 ++++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 44 insertions(+) > > > > diff --git a/test/threads.cpp b/test/threads.cpp > > index d83b5833998f..7daa76edb00e 100644 > > --- a/test/threads.cpp > > +++ b/test/threads.cpp > > @@ -8,7 +8,9 @@ > > #include <chrono> > > #include <iostream> > > #include <memory> > > +#include <pthread.h> > > #include <thread> > > +#include <time.h> > > > > #include <libcamera/base/thread.h> > > > > @@ -35,6 +37,32 @@ private: > > chrono::steady_clock::duration duration_; > > }; > > > > +class CancelThread : public Thread > > +{ > > +public: > > + CancelThread(bool &cancelled) > > + : cancelled_(cancelled) > > + { > > + } > > + > > +protected: > > + void run() > > + { > > + cancelled_ = true; > > + > > + /* Cancel the thread can call a guaranteed cancellation point. */ I meant s/can/and/. I'll turn this into /* * Cancel the thread can call a guaranteed cancellation point *(nanosleep). */ > > + pthread_cancel(pthread_self()); > > + > > + struct timespec req{ 0, 100*000*000 }; > > + nanosleep(&req, nullptr); > > Took a bit of reading to determine that nanosleep() is a cancellation > point :) > Maybe it deserves a comment as well, upto you. > > Reviewed-by: Umang Jain <umang.jain@ideasonboard.com> > > > + > > + cancelled_ = false; > > + } > > + > > +private: > > + bool &cancelled_; > > +}; > > + > > class ThreadTest : public Test > > { > > protected: > > @@ -118,6 +146,22 @@ protected: > > return TestFail; > > } > > > > + /* Test thread cleanup upon abnormal termination. */ > > + bool cancelled = false; > > + bool finished = false; > > + > > + thread = std::make_unique<CancelThread>(cancelled); > > + thread->finished.connect(this, [&finished]() { finished = true; }); > > + > > + thread->start(); > > + thread->exit(0); > > + thread->wait(chrono::milliseconds(1000)); > > + > > + if (!cancelled || !finished) { > > + cout << "Cleanup failed upon abnormal termination" << endl; > > + return TestFail; > > + } > > + > > return TestPass; > > } > >
Hi, On 10/4/22 7:43 PM, Laurent Pinchart wrote: > On Tue, Oct 04, 2022 at 07:35:42PM +0530, Umang Jain wrote: >> On 10/4/22 5:28 AM, Laurent Pinchart via libcamera-devel wrote: >>> If a thread ends abnormally (that is, without retuning normally from its >>> run() function, for instance with a direct call to pthread_cancel()), >>> thread cleanup should still be performed. Add a test to ensure this. >>> >>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >>> --- >>> test/threads.cpp | 44 ++++++++++++++++++++++++++++++++++++++++++++ >>> 1 file changed, 44 insertions(+) >>> >>> diff --git a/test/threads.cpp b/test/threads.cpp >>> index d83b5833998f..7daa76edb00e 100644 >>> --- a/test/threads.cpp >>> +++ b/test/threads.cpp >>> @@ -8,7 +8,9 @@ >>> #include <chrono> >>> #include <iostream> >>> #include <memory> >>> +#include <pthread.h> >>> #include <thread> >>> +#include <time.h> >>> >>> #include <libcamera/base/thread.h> >>> >>> @@ -35,6 +37,32 @@ private: >>> chrono::steady_clock::duration duration_; >>> }; >>> >>> +class CancelThread : public Thread >>> +{ >>> +public: >>> + CancelThread(bool &cancelled) >>> + : cancelled_(cancelled) >>> + { >>> + } >>> + >>> +protected: >>> + void run() >>> + { >>> + cancelled_ = true; >>> + >>> + /* Cancel the thread can call a guaranteed cancellation point. */ > I meant s/can/and/. I'll turn this into > > /* > * Cancel the thread can call a guaranteed cancellation point > *(nanosleep). > */ Perfect, thanks! >>> + pthread_cancel(pthread_self()); >>> + >>> + struct timespec req{ 0, 100*000*000 }; >>> + nanosleep(&req, nullptr); >> Took a bit of reading to determine that nanosleep() is a cancellation >> point :) >> Maybe it deserves a comment as well, upto you. >> >> Reviewed-by: Umang Jain <umang.jain@ideasonboard.com> >> >>> + >>> + cancelled_ = false; >>> + } >>> + >>> +private: >>> + bool &cancelled_; >>> +}; >>> + >>> class ThreadTest : public Test >>> { >>> protected: >>> @@ -118,6 +146,22 @@ protected: >>> return TestFail; >>> } >>> >>> + /* Test thread cleanup upon abnormal termination. */ >>> + bool cancelled = false; >>> + bool finished = false; >>> + >>> + thread = std::make_unique<CancelThread>(cancelled); >>> + thread->finished.connect(this, [&finished]() { finished = true; }); >>> + >>> + thread->start(); >>> + thread->exit(0); >>> + thread->wait(chrono::milliseconds(1000)); >>> + >>> + if (!cancelled || !finished) { >>> + cout << "Cleanup failed upon abnormal termination" << endl; >>> + return TestFail; >>> + } >>> + >>> return TestPass; >>> } >>>
diff --git a/test/threads.cpp b/test/threads.cpp index d83b5833998f..7daa76edb00e 100644 --- a/test/threads.cpp +++ b/test/threads.cpp @@ -8,7 +8,9 @@ #include <chrono> #include <iostream> #include <memory> +#include <pthread.h> #include <thread> +#include <time.h> #include <libcamera/base/thread.h> @@ -35,6 +37,32 @@ private: chrono::steady_clock::duration duration_; }; +class CancelThread : public Thread +{ +public: + CancelThread(bool &cancelled) + : cancelled_(cancelled) + { + } + +protected: + void run() + { + cancelled_ = true; + + /* Cancel the thread can call a guaranteed cancellation point. */ + pthread_cancel(pthread_self()); + + struct timespec req{ 0, 100*000*000 }; + nanosleep(&req, nullptr); + + cancelled_ = false; + } + +private: + bool &cancelled_; +}; + class ThreadTest : public Test { protected: @@ -118,6 +146,22 @@ protected: return TestFail; } + /* Test thread cleanup upon abnormal termination. */ + bool cancelled = false; + bool finished = false; + + thread = std::make_unique<CancelThread>(cancelled); + thread->finished.connect(this, [&finished]() { finished = true; }); + + thread->start(); + thread->exit(0); + thread->wait(chrono::milliseconds(1000)); + + if (!cancelled || !finished) { + cout << "Cleanup failed upon abnormal termination" << endl; + return TestFail; + } + return TestPass; }
If a thread ends abnormally (that is, without retuning normally from its run() function, for instance with a direct call to pthread_cancel()), thread cleanup should still be performed. Add a test to ensure this. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- test/threads.cpp | 44 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+)