Thank you Lennart for detailed explanation. I realized a few things in sd-bus behavior (and in my wrong approach to default bus -- yes I forgot about hello message) when I debugged the thing earlier today, and now your explanation cleared that up pretty nicely to me.
I see again that sd-bus design is well thought-out, I'm glad to work with it, and I hope to keep up to high design quality in c++ binding on top of it (the quality is also increasing the more I understand some details of sd-bus).
Perhaps more detailed docs on sd-bus would help a lot..
I will fix the approach to default busses on my side. This is a very special corner case in sdbus-c++ used in order to be able to create plain empty messages, which are then used as storage of Variant class, without user having to provide an existing bus connection themselves. Perhaps that could be refactored in future -- but disadvantage is that users won't be able to create self-contained Variants (they will have to provide their bus connection explicitly to create a Variant).
Thanks a lot! Take care.
On Thu, May 9, 2019, 17:02 Lennart Poettering <lennart@xxxxxxxxxxxxxx> wrote:
On Mi, 08.05.19 22:50, Stanislav Angelovič (angelovic.s@xxxxxxxxx) wrote:
> Heya,
>
> when writing sdbus-c++, we've observed that sd_bus_default_system function
> called in a fresh new thread returns a bus with initial ref count 2. We
> built our code upon that assumption -- we had to unref the bus twice when
> the thread local storage got freed, otherwise we'd have gotten
> memory leaks.
Uh, no, this was never the right thing to do.
> Now it broke, however, because in systemd v242, the initial ref count is 1.
> Is that a conscious change? Can we build upon the guarantee that it will
> always be 1? (1 actually seems reasonable, unlike 2).
Originally, in sd-bus each bus message (i.e. each sd_bus_message
object) keeps a ref on the bus connection (i.e. each sd_bus object) it
belongs to. Moreover, if a message is queued into a bus connection it
is referenced by it. Thus you have a ref cycle, as long as a mesage is
queued into a bus and not processed yet. The intention then was that
whenever you want to get rid of a bus at the end of your
thread/process you'd flush out the bus anyway (because otherwise you
lose queued but unwritten messages), at which point the cycle would be
cleared up as soon as all messages are written.
Also note, that when a bus connection is set up a "Hello" message is
immediately enqueued into it (this is required by the spec). Thus, by
default there'd be two references onto the bus connection object: the
one you own yourself as the caller, and the one from the Hello bus
message queued onto it.
To flush out messages you should use the sd_bus_flush() call. it's
blocking, and all it does is force out all queued outgoing messages
that are not written yet, thus emptying the write queue. Then, call
sd_bus_close(), to explicitly terminate the connection. This will
empty the read queue too. At this point there are no messages queued
anymore, i.e. all refs on the bus object must be held by you and not
internally. Finally, get rid of the whole thing by doing
sd_bus_unref(). Since these three calls are often combined, there's
sd_bus_flush_close_unref() to combine them into one.
Calling sd_bus_flush() is entirely optional btw, it's sufficient to just
call sd_bus_close(). However in that case and pending unwritten
messages are just forgotten, and not written to the connection socket,
and never seen by the receiving side.
Now you might wonder, why doesn't sd_bus_unref() always implicitly
call sd_bus_flush()? Reffing/unreffing should not be blocking, that's
why. Moreover, the concept of "default" busses of a thread is that
various bits and pieces running in the thread could quickly ask for
the default bus connection, do some stuff with it, enqueue a message
or two, then unref it again. Other code might then pick it up again,
enqueue some more messages on them, unref it again, and so on. Then,
when the thread is about to end, there might be a number of messages
queued: before exiting from the thread they should be flushed out, but
only then, there's no need to do so earlier. Thus, in the earlier
uses of the bus connection your'd just unref the bus at the end, but
when you are finally done with the default connection altogether youd'
use the more powerful sd_bus_flush_close_unref() instead, and do all
the work synchronously, that the earlier users didn't bother to wait
for.
Note that when an sd_bus connection is attached to an sd_event event
loop, the sd_bus_flush() + sd_bus_close() is done automatically when
the "exit" phase of the event loop is entered.
Now, as it turns out, many users of sd-bus didn't get all of the above
right: they simply unreffed the bus after use, and enver bothered to
flush out any queued messages not written yet, and thus leaked
memory. With 1b3f9dd759ca0ea215e7b89f8ce66d1b724497b9 that was
addressed: now sd-bus tries to be a bit smarter than before: if it
notices, that a bus connection is only referenced by a message queued
into it but nothing else, and that there's no chance that it can be
referenced again, we'll detect this and release the entire object
automatically, knowing that it could never correctly be flushed out
anyway, since noone owns a ref to it anymore. Or in other words: the
ref cycles are now detected, and dealt with.
What does this mean for users? Well, if you are using sd-bus correctly
not much changes: make sure you flush/close your connections at the
end of each thread, to make sure you don't lose messages you enqueued
but that were never written out. If you don't do that correctly though
it's not a memory leak anymore as before if you forget to do that.
Lennart
--
Lennart Poettering, Berlin
_______________________________________________ systemd-devel mailing list systemd-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/systemd-devel
