Re: trunk r116499: Improve dbus error handling; detect bus failure

[Daniel Colascione <dancol@dancol.org>]

This change is a bug fix --- a non-trivial bug fix, but a bug fix. 
There's no contractual behavior change.

On 02/24/2014 12:18 AM, Michael Albinus wrote:
> Daniel Colascione <dan.colascione@gmail.com> writes:
>
>> +	(dbus-handle-bus-disconnect): New function.  React to bus
>> +	disconnection signal by synthesizing dbus error for each
>> +	pending synchronous or asynchronous call.
>
> This is a new functionality. Why do you want to add it during feature
> freeze? And what is the reasoning behind? When a D-Bus bus disconnects,
> there might be more serious problems but pending method calls.

And you would propose to just ignore bus disconnects? Sending an error 
is the best we can do.

> Is it about performance, and abortion of such pending calls? When does
> it happen, that a bus disconnect for you?

It's not about performance, but since I had to restructure the code to 
handle errors anyway, I used a mechanism that doesn't involve so many 
hash table lookups.

>
>> +	(dbus-notice-synchronous-call-errors): New function.
>> +	(dbus-handle-event): Raise errors directly only when `dbus-debug'
>> +	is true, not all the time.
>
> Why that? You cannot assume that `dbus-event-error-functions' is non-nil
> (and contains `dbus-notice-synchronous-call-errors').

We still call dbus-event-error-functions.  In fact, that's how error 
propagation to synchronous calls works now.

> I still don't see what you are trying to resolve. Lisp errors are shown
> when happening in handlers, what else do you want to achieve? As usual a
> typical use case would help.

First of all, before my change, the dbus could would just hang in 
response to bus disconnects. Now calls terminate immediately with a 
reasonable error. dbus disconnects the bus on certain usage errors. Here 
is a call that results in an immediate bus disconnect:

(dbus-call-method :session "org.freedesktop.secrets" 
"/org/freedesktop/secrets/collection/login" 
"org.freedesktop.Secret.Collection" "SearchItems" '(:array (:dict-entry 
"host" ("xxxx" "xxxx")) (:dict-entry "port" "imaps")))

Additionally, I strongly dislike signaling errors from the read-event 
handler: doing that makes it easy to "leak" errors up to unrelated outer 
dbus calls. Please carefully consider cases where more than one dbus 
call might be pending on the stack at a time.

> And *if* you are applying such changes, I would expect respective doc
> changes, for example explainig the (changed) structure of the values in
> `dbus-return-values-table', or mentioning
> `dbus-notice-synchronous-call-errors' and its intended use in dbus.texi.

Neither of these symbols is public. The contractual behavior has not 
changed.