dbus_g_connection_close() ?

[Havoc Pennington]

On Sat, 2005-09-10 at 22:13 +0100, Mark McLoughlin wrote:
>  And my patch was making it even more annoying
> because you had to actually handle the Disconnected message or the
> connection wouldn't get finalized.

That sounds broken, I think I didn't fully understand your patch
then ;-)

> 	So, why do we have this requirement? I think its because we want to
> guarantee that people will always get a Disconnected message, but if the
> app drops the last ref to the connection, why would it want a
> Disconnected message? It seems to me that the Disconnected message is
> really only useful if the other side of the connection is the one that
> closed it.

Rambling on this a bit:

One reason in my mind is reentrancy. A typical place for the last unref
might be the wrapper object's finalizer (in a garbage collected
language), but even in plain C if you are invoking the disconnected
handler or other handlers while finalizing the DBusConnection then it's
pretty easy for badness to occur.

This is why GObject has the funky-ass stuff about how destroy can happen
multiple times and objects can come back to life and all that.

Threads make it all worse...

If you think about it, open/close are like malloc/free, not like
reference counting. Any non-broken code will have a clear idea about who
is supposed to open/close; who owns the "opened-ness" of the connection.

That was my suggestion about the globally shared connections
(dbus_bus_get()), which was that libdbus owns that; the only way to open
is to call dbus_bus_get(), and the only way to close is to have the
other end drop the connection, i.e. "close only happens in exceptional
circumstances" (an app could decide to close, but other users of the
shared connection would perceive this as "something very bad happened").
Thus the default behavior for system/session bus of exiting the app on
closing these connections.

Anyway, the point is that in good code occasions for open and close are
carefully thought through. The same would apply to a private or
dedicated connection of whatever kind, not just our global shared
connection; you have to figure out who owns opening and closing and what
the interface contract surrounding a particular connection is.

A separate point is that keeping a connection open due to some random
refcount I'm guessing would generally not be desirable. If your primary
"owner" of a connection (the reason for the connection's existence) goes
away, say the "talk to HAL" object or whatever it is, then you don't
want to keep queuing up messages and stuff. You might have reason to
avoid freeing the connection's memory block, but you want the connection
to go inactive.

One thing I am worried about thinking this all through is the difference
between dbus_bus_get() and dbus_connection_open(). bus_get() accesses a
global variable which strongly owns the connection; the connection has
"infinite" lifetime and that means bus_get() doesn't even have to
increment the refcount when it returns it. The app even exits if the
connection disconnects, by default.

dbus_connection_open() connection-sharing though is intended to
basically work as a cache, so that if you had an API that treated
connections as implementation details (you would just activate an
object, not open a connection) the connections would be dynamically
shared and cached as required. Cache-type semantics require incrementing
the reference count when you ask for a connection. But they also raise
the question of cache expiration, and right now the answer is broken.

 - if the caller of connection_open() calls close then other users
   of the connection will break
 - if the caller of connection_open() doesn't call close then it
   can't unref either without causing an error

So I think what we want for _shared_ connections in this case is that
the last unref does close the connection... or maybe an easier and
conceptually more right approach is that the cache owns a ref, and it
closes and unrefs when the refcount reaches 1 i.e. when the only ref is 
from the cache.

But for a private connection, the caller of connection_open_private()
should have to call close() I would think.

I think I considered calling connection_open() something like
connection_open_shared(). maybe connection_open_cached() would be even
clearer.

In any case to figure this out we definitely have at least three cases:
 - the bus_get() global connections
 - regular cached/shared connections
 - private connections

> 	Also, you ment
> ioned the thing about Java objects that close file
> descriptors doing so in an explicit close() method rather than in the
> finalizer ... isn't that because the object isn't garbage collected
> until some indefinite time later? We don't have that problem in C ...

Ah, but:
 - libdbus is designed to export to collected languages, and 
   refcounting really has some of the same indeterminism 
   anyhow
 - the Java example shows that explicit close() works fine,
   as an API design pattern

>   + The @todo and a _dbus_assert_not_reached() in unref_unlocked() 
>     basically comes down to the problem - how can you unlock a 
>     connection *after* you drop its last reference, so the function
>     doesn't really seem to make sense to me. It came up because
>     various things in last_unref() require us to not be holding
>     the lock.

It does seem a bit wrong, but the fix may be more complicated... it
seems to be used when we enter a function with the lock held, ref, drop
the lock, reacquire lock, and unref.

The problem is that this is useless unless the caller of such a function
is already holding another safety ref. So the ref/unref here has no
value. The solution might be either to be sure the caller always holds a
ref/unref and maybe give the lock-dropping function a scary name, or
make the lock-dropping function be an and_ref() function i.e. also
increment refcount as a side effect to enforce the safety-refcounting.

> 	Also, I've attached another patch with some random fixes which are more
> obvious.

In addition to the "connection watch should be held when calling this"
note you might add _unlocked() to the function name, which is the
(inconsistent) convention in this code.

Misc fixes patch looks good, close/unref patch I think I'm still
wrapping my head around, see above ramblings...

Havoc