Crash in _dbus_connection_block_pending_call

[John (J5) Palmieri]

On Tue, 2006-06-13 at 22:43 -0400, Havoc Pennington wrote:
> John (J5) Palmieri wrote:
> >     http://nouse.net/misc/nm-trace-verbose-O0-ggdb3-{a,b}
> > 
> > Ok, so I think I see whats happening now.  The pending call is most
> > likely finished in another thread by the time it reaches
> > check_for_reply_and_update_dispatch_unlocked.  The connection itself is
> > locked but not the pending call.  I think checking for connection ==
> > NULL and returning if it is will fix the issue.  Then again it might
> > cause messages to be dropped.  I'm not totally sure.
> > 
> 
> Checking connection == NULL doesn't fix the race, just makes it shorter.
> 
> Looking at the code, in the TODO where it says "API and thread safety 
> issues around pending call" it should probably say "pending call majorly 
> unthreadsafe" I guess.
> 
> I think the original idea was that you could only use each pending call 
> from a single thread, but that falls apart with connection itself using 
> pending calls internally.
> 
> It looks to me like each pending call needs its own lock.
> 
> Maybe with some cleverness the connection lock could cover all 
> outstanding PendingCall for that connection, avoiding a per-pending-call 
> lock, but this only works if pending->connection is never set to NULL. 
> Which would presumably mean the pending call holds a refcount on the 
> connection object.
> 
> If I were adding a per-pending-call lock I'd probably move the 
> DBusPendingCall struct into dbus-pending-call.c and add accessors so 
> that dbus-connection.c doesn't access the struct directly, this would 
> make it easier to verify the locking's correctness.
> 
> The per-call lock probably would show up in valgrind-type profiles as a 
> few percentage hit, though it's probably lost in the noise on a sampling 
> profile or real-world-app profile.

Dan seems to think refcounting the connection is the best option since
having two locks brings up the chances for a deadlock.  In any case I'm
going to privatize the struct and create accessor functions.  If we find
we need to provide PendingCall locking we can do that in the future
along with the refcounted connection property.  I'm going to start
working on this at GUADEC next week.
 
-- 
John (J5) Palmieri <johnp at redhat.com>