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

[Kieran Bingham]

Quoting Jacopo Mondi via libcamera-devel (2022-10-07 16:01:57)
> Hi Laurent
> 
> 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.
> 
> It just feel more natural to have a clean() call on error paths and
> not having to call clean() on a succesfull return.

If we use the name 'Cleaner' then, I'd actually agree here, as it would
be clear we're recording and collecting actions to 'clean', and
explicitly specifying 'when' to run them.

But as I said in the cover letter, I prefer the 'ScopeExit' terms, and I
think it's more likely that we'd catch bugs if we forget to 'cancel' a
cleanup/defer operation, than catch them if we forget to act on the
cleanups on a less used error path...


> > + *
> > + * 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
> >