[patch] break glib bindings by adding timeout_milliseconds

[David Zeuthen]

On Wed, 2006-03-01 at 00:31 -0500, Havoc Pennington wrote:
> Two more things you'd have to change:
>  - in dbus-pending-call.c the timeout gets clamped to a max of 6 hours. 
>    The comment there mentions some math that needs fixing for overflow
>    issues if changing this.

This should be fixable, yes?

>  - the bus has a reply timeout of 5 minutes (configurable in the conf 
>    file)

Hmm.. we should be able to change this from 5 minutes to "forever" for
the system bus at least.. clients pretty much needs to trust apps like
HAL or NetworkManager on the system bus. Yes?

> So when testing I think you probably just were exceeding the default
> timeout of 25 seconds or so when suspending, 

When passing timeout = 49 days to dbus-send it worked great for
Hibernate which took something like 2 minutes to return (swsusp in the
upstream kernel is sloow!).. 

> but not the 5 minutes.
> Either that or the 5 minute timeout doesn't work, which is another issue
> if so...

Right, so I just hibernated my box for 10 minutes and it worked fine:

 $ dbus-send --system --print-reply \
             --reply-timeout=2147483648 \
             --dest=org.freedesktop.Hal \
             /org/freedesktop/Hal/devices/computer \
             org.freedesktop.Hal.Device.SystemPowerManagement.Hibernate
 method return sender=:1.2 -> dest=:1.230
 uint32 0

So dbus-send did return as expected even though it took HAL about ten
minutes to return. So this needs some investigation. I'll try to
hibernate it overnight too to see about whether the six hour clamp is
broken too.

> Implementation aside, I don't think an INT_MAX timeout is the right
> approach. If you mean "forever" then we should just make it really be
> forever, not 49 days. Kind of tricky since there's only one -1 magic
> value - with dbus_connection_read_write_dispatch() it means forever,
> with the pending call stuff it means "default timeout"

Two options

 a. Add another sentinel, e.g. make -2 mean forever

 b. Break protocol, ABI and API of entire dbus and go for 64-bit
    timeouts

I don't really like magic numbers such as -1 or -2 so I would prefer b.
[1]. Realistically, though, we probably want to go for a. and also add
symbolic constants

 #define DBUS_TIMEOUT_DEFAULT -1
 #define DBUS_TIMEOUT_FOREVER -2

or something. Sure, it's more typing but, eh, the -1 in most uses of
dbus_connection_send_with_reply just seems to magical.

> The 6 hour max seems kind of wrong - the 5 minutes in the bus seems more
> logical, if there's going to be a max. If the user's stuff locked up and
> they started getting frustrated, it might self-recover before they
> tossed the computer out the window. I guess I was thinking the bus max
> is both configurable and a more clearly-understood use case while the
> client lib max might be used in some weird peer-to-peer situation.
> 
> Whether it's forever or 49 days, keep in mind that if you have no short
> timeout you're putting your app in the hands of the remote app; if the
> remote app never replies, you will be locked up for good. 

 1. Your remote app is broken if it never replies. This needs to be
    fixed in the remote app and papering it over with timeouts on the
    bus side is only going to confuse both users and developers.

 2. Your shiny UI client app would have to pass DBUS_TIMEOUT_FOREVER
    which is not the default behavior. If said client app doesn't
    gracefully allow the user to somehow cancel this request to a broken
    remote app... then by definition the client app is broken too.

> So don't do
> this with any untrusted app. (Arguably this means it's not such a good
> idea to have the "infinite timeout" API since it's something of a minor
> security pitfall)

I think it comes down to whether we want to support methods that can
take minutes, hours, even months (for e.g. Hibernate) to return. If we
don't we may as well hide the concept of timeouts from the application
developers completely. Perhaps this is why it was omitted in the glib
bindings.

Anyway, I still don't really see any good arguments against this and
frankly if we don't then application developers and system level
software will have to do silly things as proposed in 

 http://bugzilla.gnome.org/show_bug.cgi?id=332888#c13

and that would only complicate service and client code thus introducing
more bugs. Presently the behavior is unpredictable. Making the bus
timeout configurable is also just another source of bugs because then
distributions will get different bugs. Let us at least hard-code it to
say 5 minutes or whatever if you want it to stay.

Also note that we'll be adding more methods in HAL in the future that
potentially could easily take > 5 minutes to complete... such as methods
for formatting hard disks (a side effect of this method call is that it
will emit signals for progress, e.g. percentage complete for format
etc.).

The patch I posted simply fixes the glib dbus API to allow callers to
specify timeouts. This is also useful in weird and bizarre peer-to-peer
cases where timeouts needs to be controlled. So can I at least commit
this while we continue to argue whether the bus should have time outs?

Thanks,
David

[1] : but I'd want Eclipse with CDL that can do proper code refactoring
if I am to write the patch!