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

[Laurent Pinchart]

Hi Jacopo,

On Fri, Jan 18, 2019 at 06:00:44PM +0100, Jacopo Mondi wrote:
>  On Fri, Jan 18, 2019 at 06:17:19PM +0200, Laurent Pinchart wrote:
> > 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 :-)
> > 
>  
>  Happy to be of any help then :)
>  
> >> 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.
> > 
>  
>  Ah right, I thougth this was the definition of what a "single owner
>  object" is...
>  
>  
> >>> +   * 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"
> > 
>  I still feel like it is actually the ownership that is
>  transferred/moved not the reference itself. This is fine anyway!

When transferring ownership, you're right, but when borrowing the
object, it's the object that you borrow, not its ownership. I'll ask you
to borrow your pen, not the ownership of your pen :-)

> >>> +
> >>> +     * 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 :-)).
> > 
>  
>  Right!
>  
> >>> +     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.
>  
>  Does this point not apply in your opinion?

I missed it, sorry. I'll take this into account in v2.

> >>> +   * 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 :-)
> > 
>  
>  Ah! my spell checker complained and I didn't look it up online

You spellchecker rightfully complained, as "transfered" is documented as
"Misspelling of transferred" :-)

> >> * 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);
> > 
>  
>  I see... I like your suggestion...
>  
> > (This will likely not appear as-is in our code of course.)
>  
>  Hopefully not :)
>  
> > 
> >> 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."
>  
>  I let you decide if it is worth or not

I'll add it.

> >> 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.
>  
>  and hopefully won't try to access it :)
>  
> >>> +
> >>> +#. 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.
>  
>  Why can't you borrow a reference as "shared_ptr<> &" ? That's
>  effectively borrowing a shared reference without increasing the
>  reference count, isn't it?

You can do that too, but it's more complex. Functions that borrow a
reference shouldn't be written assuming that the object uses shared_ptr,
unique_ptr or no smart pointer, they should take a reference or pointer
to the object directly.

> >>> +   * 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.
> > 
>  
>  I'm fine, please go ahead merging this once you consider it ready.
>  
> >>> +
> >>> 
> >>> Tools
> >>> -----

-- 
Regards,

Laurent Pinchart