[PATCH] Have pending call keep a ref to its connection (fixes the NetworkManager crash)

[John (J5) Palmieri]

I opted to send TRUE and set the pending_reply = NULL.  If this breaks
apps they should have been checking for it in the first place (and now
is the time to break apps).  It is the simplest code path and easy to
fix in apps that are broken.

On Wed, 2006-07-12 at 22:10 -0400, Havoc Pennington wrote:
> John (J5) Palmieri wrote:
> > @@ -3734,6 +3734,10 @@
> >                                    "Disconnected"))
> >          {
> >            _dbus_bus_check_connection_and_unref (connection);
> > +
> > +          /* remove all active pending calls */
> > +          _dbus_hash_table_remove_all (connection->pending_replies);
> > +
> 
> Here we just delete the pending calls. But it might be nicer to call the 
> notify on all of them, with an error message, don't you think?
> Then people blocking on a pending call don't have to also watch for 
> disconnection - the pending call notifier is guaranteed to be called.
> 
> One way to do this; in the place that currently queues the 
> disconnect_message_link, also cancel all the pending call timeouts and 
> queue all the timeout errors, then queue the disconnect_message_link.
> 
> Then, be sure we warn or otherwise prevent adding new pending calls 
> while disconnected.
> 
> This should guarantee that no pending calls remain in the point above 
> (when processing the disconnected message) since it guarantees that 
> there's always a reply to all pending calls, before the disconnect 
> message arrives.
> 
> Does this make sense? I'm not sure I thought about all the angles.
> 
> >  
> >    _dbus_assert (timeout_milliseconds >= 0 || timeout_milliseconds == -1);
> >    
> > +  if (connection != NULL && !dbus_connection_get_is_connected (connection))
> > +    return NULL;
> > + 
> 
> Is calling this with a null connection really allowed? What does it mean 
> in that case?
> 
> I think the above get_is_connected() might make more sense as an 
> assertion/warning here, and have dbus-connection.c not call this 
> function if it's not connected.
> 
> It looks to me like dbus_connection_send_with_reply() needs more thought 
> about what happens if it's called while disconnected. The options:
>   - it's a return_if_fail - callers must check this (seems painful
>     for app developers, also could not be checked in a threadsafe
>     way)
>   - it silently does nothing - doesn't really work since it returns
>     the pending call object, and we'd have none to return
>   - it creates a pending call and immediately times it out - but
>     this is a disaster since it doesn't make sense to queue error
>     replies after disconnected is already received
>   - we add a DBusError and throw an error if you call while
>     disconnected... but this sounds like an annoying api break
>     and most people would just ignore the error
>   - we return a pending call with an error reply already set...
>     but then when is the notify function called? synchronously
>     when it gets set?
> 
> my best thought right now is that we return TRUE (had enough memory) but 
> return NULL for the pending call? But existing apps might crash in that 
> case.
> 
> I'm not sure this works at all for existing apps right now - pending 
> calls returned after disconnect might do something nasty like block 
> forever even??? Or maybe something sane happens to work out?
> 
> >    if (timeout_milliseconds == -1)
> >      timeout_milliseconds = _DBUS_DEFAULT_TIMEOUT_VALUE;
> >  
> > @@ -119,8 +122,12 @@
> >    
> >    pending->refcount.value = 1;
> >    pending->connection = connection;
> > +  if (pending->connection != NULL)
> > +    dbus_connection_ref (pending->connection);
> > +
> 
> (same here about null connection not allowed?)
> 
> Hmm. Maybe set up a test case for calling send_with_reply after 
> disconnect, and see what happens right now and then we can also verify 
> the new behavior we decide is right...
> 
> Havoc
> 
-- 
John (J5) Palmieri <johnp at redhat.com>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: dbus-fix-pending-call-crash-attempt2.patch
Type: text/x-patch
Size: 5691 bytes
Desc: not available
Url : http://lists.freedesktop.org/archives/dbus/attachments/20060713/b47c6a4e/dbus-fix-pending-call-crash-attempt2.bin