[libcamera-devel] [PATCH 1/4] Documentation: coding_style: Add object ownership rules

[Laurent Pinchart]

Hi Jacopo,

On Fri, Jan 18, 2019 at 03:56:08PM +0100, Jacopo Mondi wrote:
> Hi Laurent,
>    thanks for doing this!
> 
> I don't look picky here, but this is an important piece of
> documentation, and I want to make sure I got it right first...

This is exactly the kind of patch for which I expect picky reviews :-)

> On Fri, Jan 18, 2019 at 01:59:13AM +0200, Laurent Pinchart wrote:
> > Object ownership is a complex topic that can lead to many issues, from
> > memory leak to crashes. Document the rules that libcamera enforces to
> > make object ownership tracking explicit.
> >
> > This is a first version of the rules and is expected to be expanded as
> > the library is developed.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> >  Documentation/coding-style.rst | 62 ++++++++++++++++++++++++++++++++++
> >  1 file changed, 62 insertions(+)
> >
> > diff --git a/Documentation/coding-style.rst b/Documentation/coding-style.rst
> > index f8d2fdfeda8e..f77325239bfa 100644
> > --- a/Documentation/coding-style.rst
> > +++ b/Documentation/coding-style.rst
> > @@ -81,6 +81,68 @@ C++-11-specific features:
> >    overused.
> >  * Variadic class and function templates
> >
> > +Object Ownership
> > +~~~~~~~~~~~~~~~~
> > +
> > +libcamera creates and destroys many objects at runtime, for both objects
> > +internal to the library and objects exposed to the user. To guarantee proper
> > +operation without use after free, double free or memory leaks, knowing who owns
> > +each object at any time is crucial. The project has enacted a set of rules to
> > +make object ownership tracking as explicit and fool-proof as possible.
> > +
> > +In the context of this section, the terms object and instance are used
> > +interchangeably and both refer to an instance of a class. The term reference
> > +refers to both C++ references and C++ pointers in their capacity to refer to an
> > +object. Passing a reference means offering a way to a callee to obtain a
> > +reference to an object that the caller has a valid reference to. Borrowing a
> > +reference means using a reference passed by a caller without ownership transfer
> > +based on the assumption that the caller guarantees the validate of the
> 
> the validity

Fixed.

> > +reference for the duration of the operation that borrows the reference.
> 
> maybe just "that borrows it."

Fixed.

> > +
> > +#. Single Owner Objects
> > +
> > +   * By default an object has a single owner at any time.
> 
> "an object that has a"

No, I really meant to say that by default an object has a single owner
at any time, to explain that by default objects do not use shared
ownership.

> > +   * References to a single owner object can be borrowed by passing them from
> > +     the owner to the borrower, providing that
> 
> Not sure this was what you wanted to express, but it seems to that
> it would be more clear to state that "Ownership of a single owner
> object can be borrowed, providing that"

But then I can't use "reference" in the list below. How about

"A single owner object can be borrowed by passing a reference from the
owner to the borrower, providing that"

> > +
> > +     * the owner guarantees the validity of the reference for the whole duration
> > +       of borrowing, and
> > +     * the borrower doesn't access the reference after the end of the borrowing.
> > +
> > +     When borrowing from caller to callee for the duration of a function call,
> 
> "When borrowing from caller to callee"
> Can you borrow from callee to caller?

Yes you can. CameraManager::instance() does exactly that, it returns a
reference that is still owned by CameraManager can can be borrowed for
the duration of the libcamera lifetime (which should be long enough :-)).

> > +     this implies that the callee shall not keep any stored reference after it
> > +     returns. These rules applies to the callee and all the functions it calls,
> 
> rules -> apply

Fixed.

> > +     directly or indirectly.
> 
> * Ownership is borrowed by accessing the unique_ptr<> owned resources
>   in the caller and by declaring the callee function parameter as:
>   Your two points below here (const &) and pointer.
>   * const &
>   * pointers.
> 
> > +   * When the object doesn't need to be modified and may not be null, borrowed
> > +     references are passed as 'const &'.
> > +   * When the object may be modified or can be null, borrowed references are
> > +     passed as pointers. Unless otherwise specified, pointers passed to
> > +     functions are considered as borrowed references valid for the duration of
> > +     the function only.
> > +   * Single ownership is emphasized as much as possible by storing the unique
> > +     reference as a std::unique_ptr<>.
> 
> Shouldn't this be (one of) the first points?
> 
> > +   * Ownership is transfered by passing the reference as a std::unique_ptr<>.
> 
> s/transfered/transferred

The spelling "transfered" exists:
https://en.wiktionary.org/wiki/transfered. I'll still fix it :-)

>       * Ownership is transfered by passing the reference as a std::unique_ptr<>.
> or
>       * Ownership is transferred by declaring the callee functions
>         parameter as std::unique_ptr<>
>         (I would use the "by declaring the callee function parameter
>         as" in the description of owership borrowing too)

How about "Ownership transfer is expressed by passing the reference as a
std::unique_ptr<>." ? I'd like to avoid referring to function calls
here, as that's not the only way to transfer ownership:

struct Foo
{
	std::unique_ptr<Bar> ptr;
};

Foo a;
Foo b;

b.ptr = std::move(a.ptr);

(This will likely not appear as-is in our code of course.)

> Also, the use of std::unique_ptr<> as function argument implies the object
> ownership gets moved to the callee so it won't be valid in the caller
> afterwards (if not returned by the callee and re-assigned again in the
> caller). Is this worth being described or is it clear enough in the
> definition of what a unique_ptr<> is?

I think it's clear from the std::unique_ptr<> definition and the concept
of ownership transfer, but I can add the following sentence if you think
it could be useful.

"After ownership transfer the former owner has no valid reference to the
object anymore and shall not access it without obtaining a valid
reference."

> Furthermore, do you think moving ownership of a resource to a callee
> function should be marked as a corner case, or indication of an issue
> in the design, or do you see this happening often in practice?

I think it can happen. See the setEventDispatcher() patch for instance.
The function documentation states that the CameraManager takes ownership
of the dispatcher, but with the patch the std::unique_ptr<> in the
caller becomes null, ensuring that the caller will not erroneously try
to delete the dispatcher later on.

> > +
> > +#. Shared Objects
> > +
> > +   * Objects that may have multiple owners at a given time are called shared
> > +     objects. They are reference-counted and live as long as any references to
> > +     the object exist.
> > +   * Shared objects are created with std::make_shared<> or
> > +     std::allocate_shared<> an stored in an std::shared_ptr<>.
> > +   * Borrowed references to shared objects are passed with the same rules as for
> > +     single owner objects.
> 
> Yes, but in this case 'borrowing' and 'transferring' of ownership have
> different side effects on the object reference counting. I would
> state that explicitly.

Note that borrowing a reference to an object managed through a shared
pointer is done by passing a reference to the object itself, not the
shared pointer. I'll clarify this:

   * Borrowed references to shared objects are passed as references to the
     object itself, not to the std::shared_ptr<>, with the same rules as for
     single owner objects.

> > +   * Ownership is shared by creating and passing copies of any valid
> > +     std::shared_ptr<> reference. Ownership is released by destroying the
> > +     corresponding std::shared_ptr<>.
> > +
> > +.. attention:: Long term borrowing of single owner objects is allowed. Example
> > +   use cases are implementation of the singleton pattern (where the singleton
> > +   guarantees the validity of the reference forever), or returning references
> > +   to global objects whose lifetime matches the lifetime of the application. As
> > +   long term borrowing isn't marked through language constructs, it shall be
> > +   documented explicitly in details in the API.
> 
> Thanks, this note is very useful imho.
> Feel free to take in whatever you consider appropriate

Thank you for your comments. Please let me know if you're fine with the
changes I proposed above, and which ones should be included or dropped.

> > +
> >
> >  Tools
> >  -----

-- 
Regards,

Laurent Pinchart