Re: [systemd-devel] refcount of bus returned from sd_bus_default_system
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 wrote: > On Mi, 08.05.19 22:50, Stanislav Angelovič (angelovi...@gmail.com) 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 >
Re: [systemd-devel] refcount of bus returned from sd_bus_default_system
On Mi, 08.05.19 22:50, Stanislav Angelovič (angelovi...@gmail.com) 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@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/systemd-devel
[systemd-devel] refcount of bus returned from sd_bus_default_system
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. 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). Thanks, Stan. ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/systemd-devel
