[libcamera-devel] [PATCH 02/19] libcamera: bound_method: Avoid deadlock with ConnectionTypeBlocking

[Jacopo Mondi]

Hi Laurent

On Mon, Jan 20, 2020 at 01:43:37PM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Mon, Jan 20, 2020 at 12:30:20PM +0100, Jacopo Mondi wrote:
> > On Mon, Jan 20, 2020 at 02:24:20AM +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 at ideasonboard.com>
> > > Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > > ---
> > >  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.
> >
> > I find this a bit terse, but it might just be me.
>
> Note that this isn't introduced by this patch, just reformatted.
>
> > Invoke the receiver asynchronously.
>
> This duplicates the brief :-)
>
> > The called method or activated
> > slot are executed once the control returns to the receiver's thread
>
> I wouldn't mention slots here to keep the documentation agnostic of the
> upper layers.
>
> > event loop. The caller proceeds without waiting for the
> > invocation to complete.
>
> I'm not sure this would be an improvement, but it might just be me :-)
>
> > >   *
> > >   * \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
> >
> > I would move this new part to the end.
> >
> > > + * 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.
> >
> > Invoke the receiver asynchronously. The called method or activated
> > slot are executed once the control returns to the receiver's thread
> > event loop. The sender block until the receiver ...
> > If the sender and the receiver live in the same thread...
>
> It would then make the beginning of the paragraph uncorrect :-) It could
> be written
>
> "If the sender and the receiver live in different threads, invoke the
> receiver ... Otherwise, this is equivalent to ConnectionTypeDirect."
>
> Do you think that would be better ?
>


Whatever is fine, take what you consider more opportune in
> >
> > All minors though
> > Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>

Thanks
   j

>
> --
> Regards,
>
> Laurent Pinchart
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20200120/822a12c0/attachment.sig>