| Message ID | 20200118213603.16888-1-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Commit | 8034af7423e0b9f00c84eaa00cd095dbcf44f4a5 |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent, Thanks for your work. On 2020-01-18 23:36:03 +0200, Laurent Pinchart wrote: > ConnectionTypeBlocking always invokes the method through inter-thread > message passing, which results in deadlocks if the sender and receiver > live in the same thread. The deadlock can easily be avoided by turning > the invocation into a direct call in this case. Do so to make > ConnectionTypeBlocking easier to use when some of the senders live in > the same thread as the receiver while the other senders don't. > > Extend the object-invoke test to cover this usage. > > While at it reformat the documentation to avoid long \brief lines. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > src/libcamera/bound_method.cpp | 22 ++++++++++++++-------- > test/object-invoke.cpp | 20 ++++++++++++++++++++ > 2 files changed, 34 insertions(+), 8 deletions(-) > > diff --git a/src/libcamera/bound_method.cpp b/src/libcamera/bound_method.cpp > index e18c2eb4c68e..9aa59dc3678f 100644 > --- a/src/libcamera/bound_method.cpp > +++ b/src/libcamera/bound_method.cpp > @@ -35,16 +35,19 @@ namespace libcamera { > * thread. > * > * \var ConnectionType::ConnectionTypeQueued > - * \brief The receiver is invoked asynchronously in its thread when control > - * returns to the thread's event loop. The sender proceeds without waiting for > - * the invocation to complete. > + * \brief The receiver is invoked asynchronously > + * > + * Invoke the receiver asynchronously in its thread when control returns to the > + * thread's event loop. The sender proceeds without waiting for the invocation > + * to complete. > * > * \var ConnectionType::ConnectionTypeBlocking > - * \brief The receiver is invoked asynchronously in its thread when control > - * returns to the thread's event loop. The sender blocks until the receiver > - * signals the completion of the invocation. This connection type shall not be > - * used when the sender and receiver live in the same thread, otherwise > - * deadlock will occur. > + * \brief The receiver is invoked synchronously > + * > + * If the sender and the receiver live in the same thread, this is equivalent to > + * ConnectionTypeDirect. Otherwise, the receiver is invoked asynchronously in > + * its thread when control returns to the thread's event loop. The sender > + * blocks until the receiver signals the completion of the invocation. > */ > > /** > @@ -71,6 +74,9 @@ bool BoundMethodBase::activatePack(std::shared_ptr<BoundMethodPackBase> pack, > type = ConnectionTypeDirect; > else > type = ConnectionTypeQueued; > + } else if (type == ConnectionTypeBlocking) { > + if (Thread::current() == object_->thread()) > + type = ConnectionTypeDirect; > } > > switch (type) { > diff --git a/test/object-invoke.cpp b/test/object-invoke.cpp > index 8e2055ca620f..fa162c838c78 100644 > --- a/test/object-invoke.cpp > +++ b/test/object-invoke.cpp Nitpicking, to keep with our usual style should we not add a TC that fails before adding the fix? With or without this changed, Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > @@ -100,6 +100,26 @@ protected: > return TestFail; > } > > + /* > + * Test that blocking invocation is delivered directly when the > + * caller and callee live in the same thread. > + */ > + object_.reset(); > + > + object_.invokeMethod(&InvokedObject::method, > + ConnectionTypeBlocking, 42); > + > + switch (object_.status()) { > + case InvokedObject::NoCall: > + cout << "Method not invoked for main thread (blocking)" << endl; > + return TestFail; > + case InvokedObject::InvalidThread: > + cout << "Method invoked in incorrect thread for main thread (blocking)" << endl; > + return TestFail; > + default: > + break; > + } > + > /* > * Move the object to a thread and verify that auto method > * invocation is delivered in the correct thread. > -- > Regards, > > Laurent Pinchart > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Niklas, On Sun, Jan 19, 2020 at 12:29:16AM +0100, Niklas Söderlund wrote: > On 2020-01-18 23:36:03 +0200, Laurent Pinchart wrote: > > ConnectionTypeBlocking always invokes the method through inter-thread > > message passing, which results in deadlocks if the sender and receiver > > live in the same thread. The deadlock can easily be avoided by turning > > the invocation into a direct call in this case. Do so to make > > ConnectionTypeBlocking easier to use when some of the senders live in > > the same thread as the receiver while the other senders don't. > > > > Extend the object-invoke test to cover this usage. > > > > While at it reformat the documentation to avoid long \brief lines. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > src/libcamera/bound_method.cpp | 22 ++++++++++++++-------- > > test/object-invoke.cpp | 20 ++++++++++++++++++++ > > 2 files changed, 34 insertions(+), 8 deletions(-) > > > > diff --git a/src/libcamera/bound_method.cpp b/src/libcamera/bound_method.cpp > > index e18c2eb4c68e..9aa59dc3678f 100644 > > --- a/src/libcamera/bound_method.cpp > > +++ b/src/libcamera/bound_method.cpp > > @@ -35,16 +35,19 @@ namespace libcamera { > > * thread. > > * > > * \var ConnectionType::ConnectionTypeQueued > > - * \brief The receiver is invoked asynchronously in its thread when control > > - * returns to the thread's event loop. The sender proceeds without waiting for > > - * the invocation to complete. > > + * \brief The receiver is invoked asynchronously > > + * > > + * Invoke the receiver asynchronously in its thread when control returns to the > > + * thread's event loop. The sender proceeds without waiting for the invocation > > + * to complete. > > * > > * \var ConnectionType::ConnectionTypeBlocking > > - * \brief The receiver is invoked asynchronously in its thread when control > > - * returns to the thread's event loop. The sender blocks until the receiver > > - * signals the completion of the invocation. This connection type shall not be > > - * used when the sender and receiver live in the same thread, otherwise > > - * deadlock will occur. > > + * \brief The receiver is invoked synchronously > > + * > > + * If the sender and the receiver live in the same thread, this is equivalent to > > + * ConnectionTypeDirect. Otherwise, the receiver is invoked asynchronously in > > + * its thread when control returns to the thread's event loop. The sender > > + * blocks until the receiver signals the completion of the invocation. > > */ > > > > /** > > @@ -71,6 +74,9 @@ bool BoundMethodBase::activatePack(std::shared_ptr<BoundMethodPackBase> pack, > > type = ConnectionTypeDirect; > > else > > type = ConnectionTypeQueued; > > + } else if (type == ConnectionTypeBlocking) { > > + if (Thread::current() == object_->thread()) > > + type = ConnectionTypeDirect; > > } > > > > switch (type) { > > diff --git a/test/object-invoke.cpp b/test/object-invoke.cpp > > index 8e2055ca620f..fa162c838c78 100644 > > --- a/test/object-invoke.cpp > > +++ b/test/object-invoke.cpp > > Nitpicking, to keep with our usual style should we not add a TC that > fails before adding the fix? I've considered it, but this patch isn't a fix, it's an API behaviour change that I think makes the ConnectionTypeBlocking more useful (instead of a 100% chance of deadlocking in a particular case, which is documented, it changes the behaviour to avoid having to handle that in the caller). > With or without this changed, > > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > > @@ -100,6 +100,26 @@ protected: > > return TestFail; > > } > > > > + /* > > + * Test that blocking invocation is delivered directly when the > > + * caller and callee live in the same thread. > > + */ > > + object_.reset(); > > + > > + object_.invokeMethod(&InvokedObject::method, > > + ConnectionTypeBlocking, 42); > > + > > + switch (object_.status()) { > > + case InvokedObject::NoCall: > > + cout << "Method not invoked for main thread (blocking)" << endl; > > + return TestFail; > > + case InvokedObject::InvalidThread: > > + cout << "Method invoked in incorrect thread for main thread (blocking)" << endl; > > + return TestFail; > > + default: > > + break; > > + } > > + > > /* > > * Move the object to a thread and verify that auto method > > * invocation is delivered in the correct thread.
diff --git a/src/libcamera/bound_method.cpp b/src/libcamera/bound_method.cpp index e18c2eb4c68e..9aa59dc3678f 100644 --- a/src/libcamera/bound_method.cpp +++ b/src/libcamera/bound_method.cpp @@ -35,16 +35,19 @@ namespace libcamera { * thread. * * \var ConnectionType::ConnectionTypeQueued - * \brief The receiver is invoked asynchronously in its thread when control - * returns to the thread's event loop. The sender proceeds without waiting for - * the invocation to complete. + * \brief The receiver is invoked asynchronously + * + * Invoke the receiver asynchronously in its thread when control returns to the + * thread's event loop. The sender proceeds without waiting for the invocation + * to complete. * * \var ConnectionType::ConnectionTypeBlocking - * \brief The receiver is invoked asynchronously in its thread when control - * returns to the thread's event loop. The sender blocks until the receiver - * signals the completion of the invocation. This connection type shall not be - * used when the sender and receiver live in the same thread, otherwise - * deadlock will occur. + * \brief The receiver is invoked synchronously + * + * If the sender and the receiver live in the same thread, this is equivalent to + * ConnectionTypeDirect. Otherwise, the receiver is invoked asynchronously in + * its thread when control returns to the thread's event loop. The sender + * blocks until the receiver signals the completion of the invocation. */ /** @@ -71,6 +74,9 @@ bool BoundMethodBase::activatePack(std::shared_ptr<BoundMethodPackBase> pack, type = ConnectionTypeDirect; else type = ConnectionTypeQueued; + } else if (type == ConnectionTypeBlocking) { + if (Thread::current() == object_->thread()) + type = ConnectionTypeDirect; } switch (type) { diff --git a/test/object-invoke.cpp b/test/object-invoke.cpp index 8e2055ca620f..fa162c838c78 100644 --- a/test/object-invoke.cpp +++ b/test/object-invoke.cpp @@ -100,6 +100,26 @@ protected: return TestFail; } + /* + * Test that blocking invocation is delivered directly when the + * caller and callee live in the same thread. + */ + object_.reset(); + + object_.invokeMethod(&InvokedObject::method, + ConnectionTypeBlocking, 42); + + switch (object_.status()) { + case InvokedObject::NoCall: + cout << "Method not invoked for main thread (blocking)" << endl; + return TestFail; + case InvokedObject::InvalidThread: + cout << "Method invoked in incorrect thread for main thread (blocking)" << endl; + return TestFail; + default: + break; + } + /* * Move the object to a thread and verify that auto method * invocation is delivered in the correct thread.
ConnectionTypeBlocking always invokes the method through inter-thread message passing, which results in deadlocks if the sender and receiver live in the same thread. The deadlock can easily be avoided by turning the invocation into a direct call in this case. Do so to make ConnectionTypeBlocking easier to use when some of the senders live in the same thread as the receiver while the other senders don't. Extend the object-invoke test to cover this usage. While at it reformat the documentation to avoid long \brief lines. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- src/libcamera/bound_method.cpp | 22 ++++++++++++++-------- test/object-invoke.cpp | 20 ++++++++++++++++++++ 2 files changed, 34 insertions(+), 8 deletions(-)