2005-02-15 Havoc Pennington <hp@redhat.com>

	* dbus/dbus-connection.c (dbus_connection_dispatch): always
	complete a pending call, don't run filters first.

	* glib/dbus-gproxy.c (dbus_g_proxy_end_call): change to use
	dbus_pending_call_steal_reply

	* dbus/dbus-pending-call.c (dbus_pending_call_block): just call
	_dbus_connection_block_pending_call
	(dbus_pending_call_get_reply): change to steal_reply and return a
	ref

	* dbus/dbus-connection.c
	(dbus_connection_send_with_reply_and_block): port to work in terms
	of DBusPendingCall
	(_dbus_connection_block_pending_call): replace block_for_reply
	with this

7 files changed, 155 insertions, 139 deletions

diff --git a/ChangeLog b/ChangeLog
index 59821b83..5b2b085d 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,22 @@
+2005-02-15  Havoc Pennington  <hp@redhat.com>
+
+	* dbus/dbus-connection.c (dbus_connection_dispatch): always
+	complete a pending call, don't run filters first.
+
+	* glib/dbus-gproxy.c (dbus_g_proxy_end_call): change to use
+	dbus_pending_call_steal_reply
+
+	* dbus/dbus-pending-call.c (dbus_pending_call_block): just call
+	_dbus_connection_block_pending_call
+	(dbus_pending_call_get_reply): change to steal_reply and return a
+	ref
+
+	* dbus/dbus-connection.c
+	(dbus_connection_send_with_reply_and_block): port to work in terms
+	of DBusPendingCall
+	(_dbus_connection_block_pending_call): replace block_for_reply
+	with this
+
 2005-02-14  Havoc Pennington  <hp@redhat.com>
 
 	* dbus/dbus-userdb-util.c (_dbus_user_database_lookup_group):
diff --git a/dbus/dbus-connection-internal.h b/dbus/dbus-connection-internal.h
index 57e6fb0d..c8995706 100644
--- a/dbus/dbus-connection-internal.h
+++ b/dbus/dbus-connection-internal.h
@@ -84,9 +84,7 @@ DBusPendingCall*  _dbus_pending_call_new                       (DBusConnection
 void              _dbus_pending_call_notify                    (DBusPendingCall    *pending);
 void              _dbus_connection_remove_pending_call         (DBusConnection     *connection,
                                                                 DBusPendingCall    *pending);
-DBusMessage*      _dbus_connection_block_for_reply             (DBusConnection     *connection,
-                                                                dbus_uint32_t       client_serial,
-                                                                int                 timeout_milliseconds);
+void              _dbus_connection_block_pending_call          (DBusPendingCall    *pending);
 void              _dbus_pending_call_complete_and_unlock       (DBusPendingCall    *pending,
                                                                 DBusMessage        *message);
 dbus_bool_t       _dbus_connection_send_and_unlock             (DBusConnection     *connection,
diff --git a/dbus/dbus-connection.c b/dbus/dbus-connection.c
index 11c113ff..628f7a19 100644
--- a/dbus/dbus-connection.c
+++ b/dbus/dbus-connection.c
@@ -2171,6 +2171,9 @@ dbus_connection_send_with_reply (DBusConnection     *connection,
   return FALSE;
 }
 
+/* This is slightly strange since we can pop a message here without
+ * the dispatch lock.
+ */
 static DBusMessage*
 check_for_reply_unlocked (DBusConnection *connection,
                           dbus_uint32_t   client_serial)
@@ -2178,9 +2181,6 @@ check_for_reply_unlocked (DBusConnection *connection,
   DBusList *link;
 
   HAVE_LOCK_CHECK (connection);
-
-  /* popping incoming_messages requires dispatch lock */
-  _dbus_assert (connection->dispatch_acquired);
   
   link = _dbus_list_get_first_link (&connection->incoming_messages);
 
@@ -2201,53 +2201,49 @@ check_for_reply_unlocked (DBusConnection *connection,
 }
 
 /**
- * Blocks a certain time period while waiting for a reply.
- * If no reply arrives, returns #NULL.
- *
- * @todo could use performance improvements (it keeps scanning
- * the whole message queue for example) and has thread issues,
- * see comments in source.
- *
- * @todo holds the dispatch lock right now so blocks other threads
- * from reading messages. This could be fixed by having
- * dbus_connection_dispatch() and friends wake up this thread if the
- * needed reply is seen. That in turn could be done by using
- * DBusPendingCall for all replies we block for, so we track which
- * replies are interesting.
+ * Blocks until a pending call times out or gets a reply.
  *
  * Does not re-enter the main loop or run filter/path-registered
  * callbacks. The reply to the message will not be seen by
  * filter callbacks.
  *
- * @param connection the connection
- * @param client_serial the reply serial to wait for
- * @param timeout_milliseconds timeout in milliseconds or -1 for default
- * @returns the message that is the reply or #NULL if no reply
+ * Returns immediately if pending call already got a reply.
+ * 
+ * @todo could use performance improvements (it keeps scanning
+ * the whole message queue for example)
+ *
+ * @param pending the pending call we block for a reply on
  */
-DBusMessage*
-_dbus_connection_block_for_reply (DBusConnection     *connection,
-                                  dbus_uint32_t       client_serial,
-                                  int                 timeout_milliseconds)
+void
+_dbus_connection_block_pending_call (DBusPendingCall *pending)
 {
   long start_tv_sec, start_tv_usec;
   long end_tv_sec, end_tv_usec;
   long tv_sec, tv_usec;
   DBusDispatchStatus status;
+  DBusConnection *connection;
+  dbus_uint32_t client_serial;
+  int timeout_milliseconds;
 
-  _dbus_assert (connection != NULL);
-  _dbus_assert (client_serial != 0);
-  _dbus_assert (timeout_milliseconds >= 0 || timeout_milliseconds == -1);
+  _dbus_assert (pending != NULL);
+
+  if (dbus_pending_call_get_completed (pending))
+    return;
+
+  if (pending->connection == NULL)
+    return; /* call already detached */
+
+  dbus_pending_call_ref (pending); /* necessary because the call could be canceled */
   
-  if (timeout_milliseconds == -1)
-    timeout_milliseconds = _DBUS_DEFAULT_TIMEOUT_VALUE;
+  connection = pending->connection;
+  client_serial = pending->reply_serial;
 
-  /* it would probably seem logical to pass in _DBUS_INT_MAX
-   * for infinite timeout, but then math below would get
-   * all overflow-prone, so smack that down.
+  /* note that timeout_milliseconds is limited to a smallish value
+   * in _dbus_pending_call_new() so overflows aren't possible
+   * below
    */
-  if (timeout_milliseconds > _DBUS_ONE_HOUR_IN_MILLISECONDS * 6)
-    timeout_milliseconds = _DBUS_ONE_HOUR_IN_MILLISECONDS * 6;
-  
+  timeout_milliseconds = dbus_timeout_get_interval (pending->timeout);
+
   /* Flush message queue */
   dbus_connection_flush (connection);
 
@@ -2265,10 +2261,7 @@ _dbus_connection_block_for_reply (DBusConnection     *connection,
                  start_tv_sec, start_tv_usec,
                  end_tv_sec, end_tv_usec);
 
-  _dbus_connection_acquire_dispatch (connection);
-  
   /* Now we wait... */
-  /* THREAD TODO: Evil to block here and lock all other threads out of dispatch. */
   /* always block at least once as we know we don't have the reply yet */
   _dbus_connection_do_iteration_unlocked (connection,
                                           DBUS_ITERATION_DO_READING |
@@ -2285,6 +2278,16 @@ _dbus_connection_block_for_reply (DBusConnection     *connection,
 
   status = _dbus_connection_get_dispatch_status_unlocked (connection);
 
+  /* the get_completed() is in case a dispatch() while we were blocking
+   * got the reply instead of us.
+   */
+  if (dbus_pending_call_get_completed (pending))
+    {
+      _dbus_verbose ("Pending call completed by dispatch in %s\n", _DBUS_FUNCTION_NAME);
+      _dbus_connection_update_dispatch_status_and_unlock (connection, status);
+      return;
+    }
+  
   if (status == DBUS_DISPATCH_DATA_REMAINS)
     {
       DBusMessage *reply;
@@ -2293,16 +2296,17 @@ _dbus_connection_block_for_reply (DBusConnection     *connection,
       if (reply != NULL)
         {
           _dbus_verbose ("%s checked for reply\n", _DBUS_FUNCTION_NAME);
-          status = _dbus_connection_get_dispatch_status_unlocked (connection);
 
           _dbus_verbose ("dbus_connection_send_with_reply_and_block(): got reply\n");
-
-          _dbus_connection_release_dispatch (connection);
           
-          /* Unlocks, and calls out to user code */
+          _dbus_pending_call_complete_and_unlock (pending, reply);
+          dbus_message_unref (reply);
+
+          CONNECTION_LOCK (connection);
+          status = _dbus_connection_get_dispatch_status_unlocked (connection);
           _dbus_connection_update_dispatch_status_and_unlock (connection, status);
           
-          return reply;
+          return;
         }
     }
   
@@ -2310,9 +2314,13 @@ _dbus_connection_block_for_reply (DBusConnection     *connection,
   
   if (!_dbus_connection_get_is_connected_unlocked (connection))
     {
-      _dbus_connection_release_dispatch (connection);
-      CONNECTION_UNLOCK (connection);
-      return NULL;
+      /* FIXME send a "DBUS_ERROR_DISCONNECTED" instead, just to help
+       * programmers understand what went wrong since the timeout is
+       * confusing
+       */
+      
+      _dbus_pending_call_complete_and_unlock (pending, NULL);
+      return;
     }
   else if (tv_sec < start_tv_sec)
     _dbus_verbose ("dbus_connection_send_with_reply_and_block(): clock set backward\n");
@@ -2356,12 +2364,15 @@ _dbus_connection_block_for_reply (DBusConnection     *connection,
   _dbus_verbose ("dbus_connection_send_with_reply_and_block(): Waited %ld milliseconds and got no reply\n",
                  (tv_sec - start_tv_sec) * 1000 + (tv_usec - start_tv_usec) / 1000);
 
-  _dbus_connection_release_dispatch (connection);
+  _dbus_assert (!dbus_pending_call_get_completed (pending));
   
-  /* unlocks and calls out to user code */
-  _dbus_connection_update_dispatch_status_and_unlock (connection, status);
+  /* unlock and call user code */
+  _dbus_pending_call_complete_and_unlock (pending, NULL);
 
-  return NULL;
+  /* update user code on dispatch status */
+  CONNECTION_LOCK (connection);
+  status = _dbus_connection_get_dispatch_status_unlocked (connection);
+  _dbus_connection_update_dispatch_status_and_unlock (connection, status);
 }
 
 /**
@@ -2386,40 +2397,40 @@ _dbus_connection_block_for_reply (DBusConnection     *connection,
  * @returns the message that is the reply or #NULL with an error code if the
  * function fails.
  */
-DBusMessage *
+DBusMessage*
 dbus_connection_send_with_reply_and_block (DBusConnection     *connection,
                                            DBusMessage        *message,
                                            int                 timeout_milliseconds,
                                            DBusError          *error)
 {
-  dbus_uint32_t client_serial;
   DBusMessage *reply;
+  DBusPendingCall *pending;
   
   _dbus_return_val_if_fail (connection != NULL, NULL);
   _dbus_return_val_if_fail (message != NULL, NULL);
   _dbus_return_val_if_fail (timeout_milliseconds >= 0 || timeout_milliseconds == -1, FALSE);  
   _dbus_return_val_if_error_is_set (error, NULL);
   
-  if (!dbus_connection_send (connection, message, &client_serial))
+  if (!dbus_connection_send_with_reply (connection, message,
+                                        &pending, timeout_milliseconds))
     {
       _DBUS_SET_OOM (error);
       return NULL;
     }
 
-  reply = _dbus_connection_block_for_reply (connection,
-                                            client_serial,
-                                            timeout_milliseconds);
+  _dbus_assert (pending != NULL);
   
-  if (reply == NULL)
-    {
-      if (dbus_connection_get_is_connected (connection))
-        dbus_set_error (error, DBUS_ERROR_NO_REPLY, "Message did not receive a reply");
-      else
-        dbus_set_error (error, DBUS_ERROR_DISCONNECTED, "Disconnected prior to receiving a reply");
+  dbus_pending_call_block (pending);
 
-      return NULL;
-    }
-  else if (dbus_set_error_from_message (error, reply))
+  reply = dbus_pending_call_steal_reply (pending);
+  dbus_pending_call_unref (pending);
+
+  /* call_complete_and_unlock() called from pending_call_block() should
+   * always fill this in.
+   */
+  _dbus_assert (reply != NULL);
+  
+   if (dbus_set_error_from_message (error, reply))
     {
       dbus_message_unref (reply);
       return NULL;
@@ -2934,16 +2945,12 @@ dbus_connection_get_dispatch_status (DBusConnection *connection)
  *
  * @todo some FIXME in here about handling DBUS_HANDLER_RESULT_NEED_MEMORY
  *
- * @todo right now a message filter gets run on replies to a pending
- * call in here, but not in the case where we block without entering
- * the main loop. Simple solution might be to just have the pending
- * call stuff run before the filters.
- *
  * @todo FIXME what if we call out to application code to handle a
  * message, holding the dispatch lock, and the application code runs
  * the main loop and dispatches again? Probably deadlocks at the
  * moment. Maybe we want a dispatch status of DBUS_DISPATCH_IN_PROGRESS,
- * and then the GSource etc. could handle the situation?
+ * and then the GSource etc. could handle the situation? Right now
+ * our GSource is NO_RECURSE
  * 
  * @param connection the connection
  * @returns dispatch status
@@ -2979,18 +2986,12 @@ dbus_connection_dispatch (DBusConnection *connection)
   _dbus_connection_acquire_dispatch (connection);
   HAVE_LOCK_CHECK (connection);
 
-  /* This call may drop the lock during the execution (if waiting for
-   * borrowed messages to be returned) but the order of message
-   * dispatch if several threads call dispatch() is still
-   * protected by the lock, since only one will get the lock, and that
-   * one will finish the message dispatching
-   */
   message_link = _dbus_connection_pop_message_link_unlocked (connection);
   if (message_link == NULL)
     {
       /* another thread dispatched our stuff */
 
-      _dbus_verbose ("another thread dispatched message\n");
+      _dbus_verbose ("another thread dispatched message (during acquire_dispatch above)\n");
       
       _dbus_connection_release_dispatch (connection);
 
@@ -3015,24 +3016,44 @@ dbus_connection_dispatch (DBusConnection *connection)
                  dbus_message_get_member (message) :
                  "no member",
                  dbus_message_get_signature (message));
-  
-  result = DBUS_HANDLER_RESULT_NOT_YET_HANDLED;
 
+  result = DBUS_HANDLER_RESULT_NOT_YET_HANDLED;
+  
+  /* Pending call handling must be first, because if you do
+   * dbus_connection_send_with_reply_and_block() or
+   * dbus_pending_call_block() then no handlers/filters will be run on
+   * the reply. We want consistent semantics in the case where we
+   * dbus_connection_dispatch() the reply.
+   */
+  
   reply_serial = dbus_message_get_reply_serial (message);
   pending = _dbus_hash_table_lookup_int (connection->pending_replies,
                                          reply_serial);
+  if (pending)
+    {
+      _dbus_verbose ("Dispatching a pending reply\n");
+      _dbus_pending_call_complete_and_unlock (pending, message);
+      pending = NULL; /* it's probably unref'd */
+      
+      CONNECTION_LOCK (connection);
+      _dbus_verbose ("pending call completed in dispatch\n");
+      result = DBUS_HANDLER_RESULT_HANDLED;
+      goto out;
+    }
   
   if (!_dbus_list_copy (&connection->filter_list, &filter_list_copy))
     {
       _dbus_connection_release_dispatch (connection);
       HAVE_LOCK_CHECK (connection);
-
+      
       _dbus_connection_failed_pop (connection, message_link);
 
       /* unlocks and calls user code */
       _dbus_connection_update_dispatch_status_and_unlock (connection,
                                                           DBUS_DISPATCH_NEED_MEMORY);
 
+      if (pending)
+        dbus_pending_call_unref (pending);
       dbus_connection_unref (connection);
       
       return DBUS_DISPATCH_NEED_MEMORY;
@@ -3074,40 +3095,11 @@ dbus_connection_dispatch (DBusConnection *connection)
       _dbus_verbose ("No memory in %s\n", _DBUS_FUNCTION_NAME);
       goto out;
     }
-  
-  /* Did a reply we were waiting on get filtered? */
-  if (pending && result == DBUS_HANDLER_RESULT_HANDLED)
-    {
-      /* Queue the timeout immediately! */
-      if (pending->timeout_link)
-	{
-	  _dbus_connection_queue_synthesized_message_link (connection,
-							   pending->timeout_link);
-	  pending->timeout_link = NULL;
-	}
-      else
-	{
-	  /* We already queued the timeout? Then it was filtered! */
-	  _dbus_warn ("The timeout error with reply serial %d was filtered, so the DBusPendingCall will never stop pending.\n", reply_serial);
-	}
-    }
-  
-  if (result == DBUS_HANDLER_RESULT_HANDLED)
+  else if (result == DBUS_HANDLER_RESULT_HANDLED)
     {
       _dbus_verbose ("filter handled message in dispatch\n");
       goto out;
     }
-  
-  if (pending)
-    {
-      _dbus_pending_call_complete_and_unlock (pending, message);
-
-      pending = NULL;
-      
-      CONNECTION_LOCK (connection);
-      _dbus_verbose ("pending call completed in dispatch\n");
-      goto out;
-    }
 
   /* We're still protected from dispatch() reentrancy here
    * since we acquired the dispatcher
diff --git a/dbus/dbus-pending-call.c b/dbus/dbus-pending-call.c
index b69f7c3e..546f42a6 100644
--- a/dbus/dbus-pending-call.c
+++ b/dbus/dbus-pending-call.c
@@ -61,6 +61,14 @@ _dbus_pending_call_new (DBusConnection    *connection,
   if (timeout_milliseconds == -1)
     timeout_milliseconds = _DBUS_DEFAULT_TIMEOUT_VALUE;
 
+  /* it would probably seem logical to pass in _DBUS_INT_MAX for
+   * infinite timeout, but then math in
+   * _dbus_connection_block_for_reply would get all overflow-prone, so
+   * smack that down.
+   */
+  if (timeout_milliseconds > _DBUS_ONE_HOUR_IN_MILLISECONDS * 6)
+    timeout_milliseconds = _DBUS_ONE_HOUR_IN_MILLISECONDS * 6;
+  
   if (!dbus_pending_call_allocate_data_slot (&notify_user_data_slot))
     return NULL;
   
@@ -102,6 +110,8 @@ _dbus_pending_call_new (DBusConnection    *connection,
 void
 _dbus_pending_call_notify (DBusPendingCall *pending)
 {
+  _dbus_assert (!pending->completed);
+  
   pending->completed = TRUE;
 
   if (pending->function)
@@ -258,21 +268,26 @@ dbus_pending_call_get_completed (DBusPendingCall *pending)
 }
 
 /**
- * Gets the reply, or returns #NULL if none has been received yet. The
- * reference count is not incremented on the returned message, so you
- * have to keep a reference count on the pending call (or add one
- * to the message).
- *
- * @todo not thread safe? I guess it has to lock though it sucks
- * @todo maybe to make this threadsafe, it should be steal_reply(), i.e. only one thread can ever get the message
+ * Gets the reply, or returns #NULL if none has been received
+ * yet. Ownership of the reply message passes to the caller. This
+ * function can only be called once per pending call, since the reply
+ * message is tranferred to the caller.
  * 
  * @param pending the pending call
  * @returns the reply message or #NULL.
  */
 DBusMessage*
-dbus_pending_call_get_reply (DBusPendingCall *pending)
+dbus_pending_call_steal_reply (DBusPendingCall *pending)
 {
-  return pending->reply;
+  DBusMessage *message;
+  
+  _dbus_return_val_if_fail (pending->completed, NULL);
+  _dbus_return_val_if_fail (pending->reply != NULL, NULL);
+  
+  message = pending->reply;
+  pending->reply = NULL;
+
+  return message;
 }
 
 /**
@@ -292,20 +307,7 @@ dbus_pending_call_get_reply (DBusPendingCall *pending)
 void
 dbus_pending_call_block (DBusPendingCall *pending)
 {
-  DBusMessage *message;
-
-  if (dbus_pending_call_get_completed (pending))
-    return;
-
-  /* message may be NULL if no reply */
-  message = _dbus_connection_block_for_reply (pending->connection,
-                                              pending->reply_serial,
-                                              dbus_timeout_get_interval (pending->timeout));
-
-  _dbus_connection_lock (pending->connection);
-  _dbus_pending_call_complete_and_unlock (pending, message);
-  if (message)
-    dbus_message_unref (message);
+  _dbus_connection_block_pending_call (pending);
 }
 
 static DBusDataSlotAllocator slot_allocator;
diff --git a/dbus/dbus-pending-call.h b/dbus/dbus-pending-call.h
index 25ce8bad..f63fa197 100644
--- a/dbus/dbus-pending-call.h
+++ b/dbus/dbus-pending-call.h
@@ -41,7 +41,7 @@ dbus_bool_t  dbus_pending_call_set_notify    (DBusPendingCall               *pen
                                               DBusFreeFunction               free_user_data);
 void         dbus_pending_call_cancel        (DBusPendingCall               *pending);
 dbus_bool_t  dbus_pending_call_get_completed (DBusPendingCall               *pending);
-DBusMessage* dbus_pending_call_get_reply     (DBusPendingCall               *pending);
+DBusMessage* dbus_pending_call_steal_reply   (DBusPendingCall               *pending);
 void         dbus_pending_call_block         (DBusPendingCall               *pending);
 
 dbus_bool_t dbus_pending_call_allocate_data_slot (dbus_int32_t     *slot_p);
diff --git a/doc/TODO b/doc/TODO
index aff0b114..06beb93a 100644
--- a/doc/TODO
+++ b/doc/TODO
@@ -33,6 +33,8 @@ Important for 1.0
 
  - make the mutex/condvar functions private
 
+ - dbus-pending-call.c has some API and thread safety issues to review
+
 Important for 1.0 GLib Bindings
 ===
 
diff --git a/glib/dbus-gproxy.c b/glib/dbus-gproxy.c
index 1c2f4a1d..17b76d93 100644
--- a/glib/dbus-gproxy.c
+++ b/glib/dbus-gproxy.c
@@ -1386,7 +1386,7 @@ dbus_g_proxy_end_call (DBusGProxy          *proxy,
   g_return_val_if_fail (pending != NULL, FALSE);
 
   dbus_pending_call_block (DBUS_PENDING_CALL_FROM_G_PENDING_CALL (pending));
-  message = dbus_pending_call_get_reply (DBUS_PENDING_CALL_FROM_G_PENDING_CALL (pending));
+  message = dbus_pending_call_steal_reply (DBUS_PENDING_CALL_FROM_G_PENDING_CALL (pending));
 
   g_assert (message != NULL);
 
@@ -1403,6 +1403,7 @@ dbus_g_proxy_end_call (DBusGProxy          *proxy,
         }
       va_end (args);
 
+      dbus_message_unref (message);
       return TRUE;
       
     case DBUS_MESSAGE_TYPE_ERROR:
@@ -1416,6 +1417,8 @@ dbus_g_proxy_end_call (DBusGProxy          *proxy,
     }
 
  error:
+  dbus_message_unref (message);
+  
   dbus_set_g_error (error, &derror);
   dbus_error_free (&derror);
   return FALSE;