[libcamera-devel] [PATCH 2/2] libcamera: base: signal: Prevent excessive slot usage

[Kieran Bingham]

Quoting Jacopo Mondi (2022-01-07 15:01:32)
> Hi Kieran,
> 
> On Fri, Jan 07, 2022 at 12:55:06PM +0000, Kieran Bingham wrote:
> > Slots are not expected to be connected to lots of signals. Our normal
> 
> Isn't it the other way around ?
> "Signals are not expected to be connected to a lots of slots" :)

I ... er ... um ... probably? maybe ;-)

> 
> > use case is a one to one mapping with a signal expected to call one
> > slot.
> >
> > While we support multiple slots, excessive use is likely the result of a
> > bug indicating a failure to disconnect slots or repeated connection of a
> > slot which can lead to memory leaks and poor performance when deleting
> > the Object.
> >
> > Assert that a maximum of 8 slots are connected which will catch any bug
> > and still allow some multiple slot usage. If this limit is ever met it
> > can be extended and considered at that time.
> 
> I wish we had WARN_ONCE().

We could add that, but in this instance, I really want the assertion to
break things, otherwise for instance in CTS it woudl just keep running -
and woudl require someone to manually go through the logs.

Maybe it would be noisy enough to get noticed ... but maybe not...


> 
> >
> > Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > ---
> >
> > Note that this patch causes the CTS tests to fail as they hit this
> > assertion.
> >
> > It is not yet investigated if we really need more slots or if the
> > assertion has already caught a slot-leak, but posting for early review
> > anyway.

Of course the fact that it breaks CTS currently means this patch can't
go in until it's resolved anyway.



> >
> >  src/libcamera/base/signal.cpp | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> >
> > diff --git a/src/libcamera/base/signal.cpp b/src/libcamera/base/signal.cpp
> > index 9df45d079a42..7e6a23cc909d 100644
> > --- a/src/libcamera/base/signal.cpp
> > +++ b/src/libcamera/base/signal.cpp
> > @@ -7,6 +7,7 @@
> >
> >  #include <libcamera/base/signal.h>
> >
> > +#include <libcamera/base/log.h>
> >  #include <libcamera/base/mutex.h>
> >
> >  /**
> > @@ -35,6 +36,13 @@ void SignalBase::connect(BoundMethodBase *slot)
> >       if (object)
> >               object->connect(this);
> >       slots_.push_back(slot);
> > +
> > +     /*
> > +      * Slots are not expected to be used to connect many items. If we exceed
> > +      * a reasonable amount, presume that there is a bug and fail fast on
> > +      * debug builds.
> > +      */
> > +     ASSERT(slots_.size() < 8);
> 
> That's indeed a useful safety check when developing. I think it's
> worth having it in.

Thanks, I agree of course (once we find/fix the associated CTS bug)

> 
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> 
> Thanks
>   j
> 
> >  }
> >
> >  void SignalBase::disconnect(Object *object)
> > --
> > 2.32.0
> >