[libcamera-devel] [PATCH 1/2] libcamera: utils: Add Cleaner class

[Laurent Pinchart]

Hi Jacopo,

On Fri, Oct 07, 2022 at 05:01:57PM +0200, Jacopo Mondi wrote:
> On Tue, Oct 04, 2022 at 05:18:41AM +0300, Laurent Pinchart via libcamera-devel wrote:
> > The Cleaner class is a simple object that performs user-provided actions
> > upon destruction. As its name implies, it is meant to simplify cleanup
> > tasks in error handling paths.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> >  include/libcamera/base/utils.h |  13 ++++
> >  src/libcamera/base/utils.cpp   | 130 +++++++++++++++++++++++++++++++++
> >  2 files changed, 143 insertions(+)
> >
> > diff --git a/include/libcamera/base/utils.h b/include/libcamera/base/utils.h
> > index ed88b7163770..0de80eb89d15 100644
> > --- a/include/libcamera/base/utils.h
> > +++ b/include/libcamera/base/utils.h
> > @@ -9,6 +9,7 @@
> >
> >  #include <algorithm>
> >  #include <chrono>
> > +#include <functional>
> >  #include <iterator>
> >  #include <memory>
> >  #include <ostream>
> > @@ -381,6 +382,18 @@ struct defopt_t {
> >
> >  constexpr details::defopt_t defopt;
> >
> > +class Cleaner
> > +{
> > +public:
> > +	~Cleaner();
> > +
> > +	void defer(std::function<void()> &&action);
> > +	void clear();
> > +
> > +private:
> > +	std::vector<std::function<void()>> actions_;
> > +};
> > +
> >  } /* namespace utils */
> >
> >  #ifndef __DOXYGEN__
> > diff --git a/src/libcamera/base/utils.cpp b/src/libcamera/base/utils.cpp
> > index 9cd6cb197243..45e115b7031e 100644
> > --- a/src/libcamera/base/utils.cpp
> > +++ b/src/libcamera/base/utils.cpp
> > @@ -484,6 +484,136 @@ std::string toAscii(const std::string &str)
> >   * default-constructed T.
> >   */
> >
> > +/**
> > + * \class Cleaner
> > + * \brief An object that performs actions upon destruction
> > + *
> > + * The Cleaner class is a simple object that performs user-provided actions
> > + * upon destruction. As its name implies, it is meant to simplify cleanup tasks
> > + * in error handling paths.
> > + *
> > + * When the code flow performs multiple sequential actions that each need a
> > + * corresponding cleanup action, error handling quickly become tedious:
> > + *
> > + * \code{.cpp}
> > + * {
> > + * 	int ret = allocateMemory();
> > + * 	if (ret)
> > + * 		return ret;
> > + *
> > + * 	ret = startProducder();
> > + * 	if (ret) {
> > + * 		freeMemory();
> > + * 		return ret;
> > + * 	}
> > + *
> > + * 	ret = startConsumer();
> > + * 	if (ret) {
> > + * 		stopProducer();
> > + * 		freeMemory();
> > + * 		return ret;
> > + * 	}
> > + *
> > + * 	return 0;
> > + * }
> > + * \endcode
> > + *
> > + * This is prone to programming mistakes, as cleanup actions can easily be
> > + * forgotten or ordered incorrectly. One strategy to simplify error handling is
> > + * to use goto statements:
> > + *
> > + * \code{.cpp}
> > + * {
> > + * 	int ret = allocateMemory();
> > + * 	if (ret)
> > + * 		return ret;
> > + *
> > + * 	ret = startProducder();
> > + * 	if (ret)
> > + * 		goto error_free;
> > + *
> > + * 	ret = startConsumer();
> > + * 	if (ret)
> > + * 		goto error_stop;
> > + *
> > + * 	return 0;
> > + *
> > + * error_stop:
> > + * 	stopProducer();
> > + * error_free:
> > + * 	freeMemory();
> > + * 	return ret;
> > + * }
> > + * \endcode
> > + *
> > + * While this may be considered better, this solution is still quite
> > + * error-prone. Beside the risk of picking the wrong error label, the error
> > + * handling logic is separated from the normal code flow, which increases the
> > + * risk of error when refactoring the code. Additionally, C++ doesn't allow
> > + * goto statements to jump over local variable declarations, which can make
> > + * usage of this pattern more difficult.
> > + *
> > + * The Cleaner class solves these issues by allowing code that requires cleanup
> > + * actions to be grouped with its corresponding deferred error handling code:
> > + *
> > + * \code{.cpp}
> > + * {
> > + * 	Cleaner cleaner;
> > + *
> > + * 	int ret = allocateMemory();
> > + * 	if (ret)
> > + * 		return ret;
> > + *
> > + * 	cleaner.defer([&]() { freeMemory(); });
> > + *
> > + * 	ret = startProducder();
> > + * 	if (ret)
> > + * 		return ret;
> > + *
> > + * 	cleaner.defer([&]() { stopProducer(); });
> > + *
> > + * 	ret = startConsumer();
> > + * 	if (ret)
> > + * 		return ret;
> > + *
> > + * 	cleaner.clear();
> > + * 	return 0;
> > + * }
> > + * \endcode
> 
> I just wonder if
> 
> {
> 	Cleaner cleaner;
> 
> 	int ret = allocateMemory();
> 	if (ret)
> 		return ret;
> 
> 	cleaner.defer([&]() { freeMemory(); });
> 
> 	ret = startProducder();
> 	if (ret) {
>                 cleaner.clean();
> 		return ret;
>         }
> 
> 	cleaner.defer([&]() { stopProducer(); });
> 
> 	ret = startConsumer();
> 	if (ret) {
>                 cleaner.clean();
> 		return ret;
>         }
> 
> 	return 0;
> }
> 
> 
> IOW make Claner::clean() execute the cleaning actions while
> destruction is a no-op.

I've thought about it, and I like the idea of having no additional
function call in the success path. However, that brings other issues:

- There are usually more error paths than success paths, so there will
  be more clean() calls needed, with more risks of forgetting one of
  them.

- Error paths are less tested. A missing clear() in the success path
  will likely be detected very quickly (as all the deferred cleaner
  actions will be executed), while a missing clean() in one error path
  has a much higher chance to end up not being detected for a long time.

Another option would be to have dedicated calls for both the error and
success paths, and warn loudly if the object is destructed without any
of these functions having been called. It will ensure we get a warning
the first time an incorrect error path is taken, but we'll still have
risks of missing calls in error paths being detected.

Yet another option is to hook up the Cleaner class in the return
statement itself, writing

	return cleaner.return(ret);

ret == 0 would be considered a success and ret != 0 an error. Again,
nothing will guarantee that a stray

	return ret;

gets added to an error path, but it may be easier to visually catch the
offending code during review (not entirely sure about that).

Yet another option would be to change the function to return a Result
instead of an int, with the Cleaner::return() (or Cleaner::result(),
name bikeshading is allowed) function returning a Result. Here again,
someone may create a Result directly, so it may not help much.

If we were using exceptions the Cleaner destructor could differentiate
between the success and failure cases more easily. I can't see another
elegant method to achieve the same with error values, but maybe I'm
missing something.

> It just feel more natural to have a clean() call on error paths and
> not having to call clean() on a succesfull return.

Note that it's "clear", not "clean", in the success path. I think I'll
rename it to release() to avoid confusion, and match
https://en.cppreference.com/w/cpp/experimental/scope_fail.

> > + *
> > + * Deferred error handlers are executed when the Cleaner instance is destroyed,
> > + * in the reverse order of their addition.
> > + */
> > +
> > +Cleaner::~Cleaner()
> > +{
> > +	for (const auto &action : utils::reverse(actions_))
> > +		action();
> > +}
> > +
> > +/**
> > + * \brief Add a deferred cleanup action
> > + * \param[in] action The cleanup action
> > + *
> > + * Add a deferred cleanup action to the Cleaner. Cleanup actions will be called
> > + * upon destruction in the reverse order of their addition.
> > + */
> > +void Cleaner::defer(std::function<void()> &&action)
> > +{
> > +	actions_.push_back(std::move(action));
> > +}
> > +
> > +/**
> > + * \brief Clear all deferred cleanup actions
> > + *
> > + * This function should be called in the success path to clear all deferred
> > + * error cleanup actions.
> > + */
> > +void Cleaner::clear()
> > +{
> > +	actions_.clear();
> > +}
> > +
> >  } /* namespace utils */
> >
> >  #ifndef __DOXYGEN__

-- 
Regards,

Laurent Pinchart