dbus-cplusplus-devel

[dbus-cplusplus-devel] Server and closing connections

[Ignacy G. <sf...@qu...>]

Hi,

I'm playing with dbus-c++ for two days now and I stumbled upon a particular
problem related to using DBus::Server.

It seems that this part of the code needs some work and that most of the
people simply don't care, since, please correct me if I'm wrong, it's used
only in peer-to-peer communications (i.e. not going through dbus-daemon).

Of course I could for now go along with using dbus-daemon and not care about
DBus::Server that much, but I'm still wondering how to make that thing work
right.

A major problem with DBus::Server is the way DBus::Connection objects are
dealt with and more generally the fact that there apparently is no clean way
of cleaning up stuff on client disconnection.  As for DBus::Connection
instances, they obviously are never removed from the
DBus::Server::Private::connections list.

The thing is that after looking into the libdbus API and having searched
through the mailing list archives, it seems to me that there is a deeper
problem with detecting client disconnections.  A watch does not hold any
reference to the connection it's associated with and relying on connections
(and also watches) to have an associated file descriptor (that might be used
to match the one with the other) is not a good way to go, according to the API
docs (a connection is not necessarily associated with a file descriptor).  So
at no point is there a way to associate a watch to its connection, so as to
check whether it's still connected after a watch handling, or something like
that.

Do you see any way of achieving this?

Ignacy

-- 
/* This is not a comment */

Re: [dbus-cplusplus-devel] Server and closing connections

[P. D. <sh...@gm...>]

Hi,

On Wed, Sep 3, 2008 at 10:50 PM, Ignacy Gawedzki <sf...@qu...> wrote:
> Hi,
>
> I'm playing with dbus-c++ for two days now and I stumbled upon a particular
> problem related to using DBus::Server.
>
> It seems that this part of the code needs some work and that most of the
> people simply don't care, since, please correct me if I'm wrong, it's used
> only in peer-to-peer communications (i.e. not going through dbus-daemon).
>
to be honest, as far as I know, it's not used at all

> Of course I could for now go along with using dbus-daemon and not care about
> DBus::Server that much, but I'm still wondering how to make that thing work
> right.
>
> A major problem with DBus::Server is the way DBus::Connection objects are
> dealt with and more generally the fact that there apparently is no clean way
> of cleaning up stuff on client disconnection.  As for DBus::Connection
> instances, they obviously are never removed from the
> DBus::Server::Private::connections list.
>
right

> The thing is that after looking into the libdbus API and having searched
> through the mailing list archives, it seems to me that there is a deeper
> problem with detecting client disconnections.  A watch does not hold any
> reference to the connection it's associated with and relying on connections
> (and also watches) to have an associated file descriptor (that might be used
> to match the one with the other) is not a good way to go, according to the API
> docs (a connection is not necessarily associated with a file descriptor).  So
> at no point is there a way to associate a watch to its connection, so as to
> check whether it's still connected after a watch handling, or something like
> that.
>
> Do you see any way of achieving this?
>
if I understand the question correctly, the internal data pointer
which is now used to store the
Dispatcher associated with the Watch could be replaced with a pointer
to the Connection owning the watch
http://dbus.freedesktop.org/doc/dbus/api/html/structDBusWatch.html#ec763277e3fd3a3fbf34ea5bd7dda617
(the dispatcher will still be accessible through the connection object)

then, a special case to handle a closing connection could be added to
DBus::Watch::handle()
(checking for POLLHUP|POLLERR)

out of curiosity, I've looked at how the Qt people do it but
apparently they didn't implement it either
http://lists.kde.org/?l=kde-devel&m=120185882521538&w=2

> Ignacy
>
Paolo

Re: [dbus-cplusplus-devel] Server and closing connections

[Ignacy G. <sf...@qu...>]

On Thu, Sep 04, 2008 at 12:17:42AM +0200, thus spake P. Durante:
> Hi,
> 
> On Wed, Sep 3, 2008 at 10:50 PM, Ignacy Gawedzki <sf...@qu...> wrote:
> > Hi,
> >
> > I'm playing with dbus-c++ for two days now and I stumbled upon a particular
> > problem related to using DBus::Server.
> >
> > It seems that this part of the code needs some work and that most of the
> > people simply don't care, since, please correct me if I'm wrong, it's used
> > only in peer-to-peer communications (i.e. not going through dbus-daemon).
> >
> to be honest, as far as I know, it's not used at all
> 
> > Of course I could for now go along with using dbus-daemon and not care about
> > DBus::Server that much, but I'm still wondering how to make that thing work
> > right.
> >
> > A major problem with DBus::Server is the way DBus::Connection objects are
> > dealt with and more generally the fact that there apparently is no clean way
> > of cleaning up stuff on client disconnection.  As for DBus::Connection
> > instances, they obviously are never removed from the
> > DBus::Server::Private::connections list.
> >
> right
> 
> > The thing is that after looking into the libdbus API and having searched
> > through the mailing list archives, it seems to me that there is a deeper
> > problem with detecting client disconnections.  A watch does not hold any
> > reference to the connection it's associated with and relying on connections
> > (and also watches) to have an associated file descriptor (that might be used
> > to match the one with the other) is not a good way to go, according to the API
> > docs (a connection is not necessarily associated with a file descriptor).  So
> > at no point is there a way to associate a watch to its connection, so as to
> > check whether it's still connected after a watch handling, or something like
> > that.
> >
> > Do you see any way of achieving this?
> >
> if I understand the question correctly, the internal data pointer
> which is now used to store the
> Dispatcher associated with the Watch could be replaced with a pointer
> to the Connection owning the watch
> http://dbus.freedesktop.org/doc/dbus/api/html/structDBusWatch.html#ec763277e3fd3a3fbf34ea5bd7dda617
> (the dispatcher will still be accessible through the connection object)
> 
> then, a special case to handle a closing connection could be added to
> DBus::Watch::handle()
> (checking for POLLHUP|POLLERR)
> 
> out of curiosity, I've looked at how the Qt people do it but
> apparently they didn't implement it either
> http://lists.kde.org/?l=kde-devel&m=120185882521538&w=2

After having looked at that again and more from the libdbus API standpoint, I
have noticed that in any case the disconnection should be noticed and that in
fact, in dbus-c++, the Connection::Private::do_dispatch() method does check
whether the connections is terminated (by a call to
dbus_connection_get_is_connected()) and calls
Connection::Private::detach_server() in that case.

It seems there are more serious issues with the lifetime management of
Connection::Private instances, which are created but never freed it seems.
Before I can go further with my initial problems, I must resolve these.

-- 
 "The whole problem with the world is that fools and fanatics are
   always so certain of themselves, and wiser people so full of doubts."
                                                 - Bertrand Russell

Re: [dbus-cplusplus-devel] Server and closing connections

[P. D. <sh...@gm...>]

On Thu, Sep 4, 2008 at 5:42 PM, Ignacy Gawedzki <sf...@qu...> wrote:
> On Thu, Sep 04, 2008 at 12:17:42AM +0200, thus spake P. Durante:
>> Hi,
>>
>> On Wed, Sep 3, 2008 at 10:50 PM, Ignacy Gawedzki <sf...@qu...> wrote:
>> > Hi,
>> >
>> > I'm playing with dbus-c++ for two days now and I stumbled upon a particular
>> > problem related to using DBus::Server.
>> >
>> > It seems that this part of the code needs some work and that most of the
>> > people simply don't care, since, please correct me if I'm wrong, it's used
>> > only in peer-to-peer communications (i.e. not going through dbus-daemon).
>> >
>> to be honest, as far as I know, it's not used at all
>>
>> > Of course I could for now go along with using dbus-daemon and not care about
>> > DBus::Server that much, but I'm still wondering how to make that thing work
>> > right.
>> >
>> > A major problem with DBus::Server is the way DBus::Connection objects are
>> > dealt with and more generally the fact that there apparently is no clean way
>> > of cleaning up stuff on client disconnection.  As for DBus::Connection
>> > instances, they obviously are never removed from the
>> > DBus::Server::Private::connections list.
>> >
>> right
>>
>> > The thing is that after looking into the libdbus API and having searched
>> > through the mailing list archives, it seems to me that there is a deeper
>> > problem with detecting client disconnections.  A watch does not hold any
>> > reference to the connection it's associated with and relying on connections
>> > (and also watches) to have an associated file descriptor (that might be used
>> > to match the one with the other) is not a good way to go, according to the API
>> > docs (a connection is not necessarily associated with a file descriptor).  So
>> > at no point is there a way to associate a watch to its connection, so as to
>> > check whether it's still connected after a watch handling, or something like
>> > that.
>> >
>> > Do you see any way of achieving this?
>> >
>> if I understand the question correctly, the internal data pointer
>> which is now used to store the
>> Dispatcher associated with the Watch could be replaced with a pointer
>> to the Connection owning the watch
>> http://dbus.freedesktop.org/doc/dbus/api/html/structDBusWatch.html#ec763277e3fd3a3fbf34ea5bd7dda617
>> (the dispatcher will still be accessible through the connection object)
>>
>> then, a special case to handle a closing connection could be added to
>> DBus::Watch::handle()
>> (checking for POLLHUP|POLLERR)
>>
>> out of curiosity, I've looked at how the Qt people do it but
>> apparently they didn't implement it either
>> http://lists.kde.org/?l=kde-devel&m=120185882521538&w=2
>
> After having looked at that again and more from the libdbus API standpoint, I
> have noticed that in any case the disconnection should be noticed and that in
> fact, in dbus-c++, the Connection::Private::do_dispatch() method does check
> whether the connections is terminated (by a call to
> dbus_connection_get_is_connected()) and calls
> Connection::Private::detach_server() in that case.
>
> It seems there are more serious issues with the lifetime management of
> Connection::Private instances, which are created but never freed it seems.
> Before I can go further with my initial problems, I must resolve these.
>
do you have any evidence? dbus_connection_unref is called in the
destructor for this very reason, and calls to ref/unref are balanced
so to keep the Connection::Private (and the internal DBusConnection)
around for as long as there's at least a DBus::Connection referring to
it

Re: [dbus-cplusplus-devel] Server and closing connections

[Ignacy G. <sf...@qu...>]

On Thu, Sep 04, 2008 at 05:59:18PM +0200, thus spake P. Durante:
> On Thu, Sep 4, 2008 at 5:42 PM, Ignacy Gawedzki <sf...@qu...> wrote:
> > It seems there are more serious issues with the lifetime management of
> > Connection::Private instances, which are created but never freed it seems.
> > Before I can go further with my initial problems, I must resolve these.
> >
> do you have any evidence? dbus_connection_unref is called in the
> destructor for this very reason, and calls to ref/unref are balanced
> so to keep the Connection::Private (and the internal DBusConnection)
> around for as long as there's at least a DBus::Connection referring to
> it

Uuuh, if you mean evidence for Connection::Private instances not being freed,
yes.  They are new'd in Connection::Connection(const char*, bool) but I see no
corresponding delete anywhere in the sources.

-- 
-= Best Viewed Using [INLINE] =-

Re: [dbus-cplusplus-devel] Server and closing connections

[P. D. <sh...@gm...>]

On Thu, Sep 4, 2008 at 8:08 PM, Ignacy Gawedzki <sf...@qu...> wrote:
> On Thu, Sep 04, 2008 at 05:59:18PM +0200, thus spake P. Durante:
>> On Thu, Sep 4, 2008 at 5:42 PM, Ignacy Gawedzki <sf...@qu...> wrote:
>> > It seems there are more serious issues with the lifetime management of
>> > Connection::Private instances, which are created but never freed it seems.
>> > Before I can go further with my initial problems, I must resolve these.
>> >
>> do you have any evidence? dbus_connection_unref is called in the
>> destructor for this very reason, and calls to ref/unref are balanced
>> so to keep the Connection::Private (and the internal DBusConnection)
>> around for as long as there's at least a DBus::Connection referring to
>> it
>
> Uuuh, if you mean evidence for Connection::Private instances not being freed,
> yes.  They are new'd in Connection::Connection(const char*, bool) but I see no
> corresponding delete anywhere in the sources.
>
since the private data is in a reference counted smart pointer
(Connection::_pvt), delete is called automatically when the last
Connection referring to that Private data is deallocated

Re: [dbus-cplusplus-devel] Server and closing connections

[Ignacy G. <sf...@qu...>]

On Thu, Sep 04, 2008 at 08:55:36PM +0200, thus spake P. Durante:
> On Thu, Sep 4, 2008 at 8:08 PM, Ignacy Gawedzki <sf...@qu...> wrote:
> > Uuuh, if you mean evidence for Connection::Private instances not being freed,
> > yes.  They are new'd in Connection::Connection(const char*, bool) but I see no
> > corresponding delete anywhere in the sources.
> >
> since the private data is in a reference counted smart pointer
> (Connection::_pvt), delete is called automatically when the last
> Connection referring to that Private data is deallocated

You know that's funny, because I was about to suggest the use of a smart
pointer, to do just that.  I've totally overlooked that it was already a smart
pointer. =)  Sorry for the bother then, my bad.

-- 
To err is human, to purr feline.

Re: [dbus-cplusplus-devel] Server and closing connections (patch)

[Ignacy G. <sf...@qu...>]

On Thu, Sep 04, 2008 at 10:37:40PM +0200, thus spake Ignacy Gawedzki:
> On Thu, Sep 04, 2008 at 08:55:36PM +0200, thus spake P. Durante:
> > since the private data is in a reference counted smart pointer
> > (Connection::_pvt), delete is called automatically when the last
> > Connection referring to that Private data is deallocated
> 
> You know that's funny, because I was about to suggest the use of a smart
> pointer, to do just that.  I've totally overlooked that it was already a smart
> pointer. =)  Sorry for the bother then, my bad.

Back to the original issue: here's a patch.

What this does is basically add a sort of "bound callback" to each connection
that is filled during Server::on_new_conn_cb() and provides everything but the
Connection::Private pointer for Connection::Private::do_dispatch() to call.

The idea is to make the server handle the fact that the connection is
disconnected.  First "privately" in Server::Private::on_rem_connection() which
in turn calls Server::on_rem_connection() using the pointer to the Server's
instance that has been bounded in the callback.  By the way, is there any good
reason for Callback::_cb not to be a reference instead of a pointer?

I've also added specialisations in util.h for the case a callback doesn't need
any argument.  I wrote this for an earlier version that's been changed since,
but I decided to leave it there so it won't harm.

One last question: wouldn't it be better to avoid linear search during
detach_server()?  An iterator to the added list element could be kept
somewhere in Connection::Private and could be used by
Server::Private::on_rem_connection() to remove the element in O(1).

Please tell me what you think.

Ignacy

-- 
-= Best Viewed Using [INLINE] =-

Re: [dbus-cplusplus-devel] Server and closing connections (corrected patch, sorry)

[Ignacy G. <sf...@qu...>]

It's a bit late I guess...

Here comes the patch that actually should work.

-- 
To err is human, to purr feline.

Re: [dbus-cplusplus-devel] Server and closing connections (corrected patch, sorry)

[Ignacy G. <sf...@qu...>]

On Fri, Sep 05, 2008 at 03:42:23AM +0200, thus spake Ignacy Gawedzki:
> It's a bit late I guess...
> 
> Here comes the patch that actually should work.

It's definitely too late.

-- 
If you're not living on the edge, you're taking up too much space.