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

[Jacopo Mondi]

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" :)

> 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().

>
> 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.
>
>  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.

Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>

Thanks
  j

>  }
>
>  void SignalBase::disconnect(Object *object)
> --
> 2.32.0
>