Attachment 36784 Details for Bug 28920 – Rework cancellable handling in tp_contact_request_contact_info_async()

[patch] Rework cancellable handling in tp_contact_request_contact_info_async()

0001-Rework-cancellable-handling-in-tp_contact_request_co.patch (text/plain), 4.60 KB, created by Philip Withnall on 2010-07-06 03:37:42 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Philip Withnall

Created: 2010-07-06 03:37:42 UTC

Size: 4.60 KB

patch

obsolete

>From 49b35489b487f82e988e0b41c826e73a53cc2d0b Mon Sep 17 00:00:00 2001
>From: Philip Withnall <philip.withnall@collabora.co.uk>
>Date: Mon, 5 Jul 2010 17:11:34 +0100
>Subject: [PATCH] Rework cancellable handling in tp_contact_request_contact_info_async()
>
>The call to
>tp_cli_connection_interface_contact_info_call_request_contact_info() returns
>synchronously in the error condition, meaning the cancellable connection is
>redundant.
>
>The cancellable doesn't have to be disconnected from in the cancelled
>callback, so the idle function for achiving this without deadlock can be
>removed.
>
>Also make the async operation complete in an idle callback rather than
>synchronously in this case.
>
>Fixes: bfo#28920
>---
> telepathy-glib/contact.c |   49 +++++++++++++--------------------------------
> 1 files changed, 14 insertions(+), 35 deletions(-)
>
>diff --git a/telepathy-glib/contact.c b/telepathy-glib/contact.c
>index 1622a81..c54e7c6 100644
>--- a/telepathy-glib/contact.c
>+++ b/telepathy-glib/contact.c
>@@ -2501,7 +2501,6 @@ typedef struct
>   TpProxyPendingCall *call;
>   GCancellable *cancellable;
>   gulong cancelled_id;
>-  guint cancel_idle_id;
> } ContactInfoRequestData;
> 
> static void
>@@ -2510,10 +2509,6 @@ contact_info_request_data_disconnect_cancellable (ContactInfoRequestData *data)
>   if (data->cancelled_id != 0)
>     g_cancellable_disconnect (data->cancellable, data->cancelled_id);
>   data->cancelled_id = 0;
>-
>-  if (data->cancel_idle_id != 0)
>-    g_source_remove (data->cancel_idle_id);
>-  data->cancel_idle_id = 0;
> }
> 
> static void
>@@ -2541,7 +2536,9 @@ contact_info_request_cb (TpConnection *connection,
>   ContactInfoRequestData *data = user_data;
>   TpContact *self = data->contact;
> 
>-  /* At this point it's too late to cancel the operation */
>+  /* At this point it's too late to cancel the operation. This will block
>+   * until the signal handler has finished if it's already running, so we're
>+   * guaranteed to never be in a partially-cancelled state after this call. */
>   contact_info_request_data_disconnect_cancellable (data);
> 
>   if (error != NULL)
>@@ -2554,43 +2551,26 @@ contact_info_request_cb (TpConnection *connection,
>       contact_maybe_set_info (self, contact_info);
>     }
> 
>-  g_simple_async_result_complete (data->result);
>+  g_simple_async_result_complete_in_idle (data->result);
>   data->call = NULL;
> }
> 
>-static gboolean
>-contact_info_request_cancelled_idle_cb (gpointer user_data)
>+static void
>+contact_info_request_cancelled_cb (GCancellable *cancellable,
>+    ContactInfoRequestData *data)
> {
>-  ContactInfoRequestData *data = user_data;
>   GError *error = NULL;
> 
>-  data->cancel_idle_id = 0;
>-
>-  if (!g_cancellable_set_error_if_cancelled (data->cancellable, &error))
>-    return FALSE;
>+  g_assert (g_cancellable_set_error_if_cancelled (data->cancellable, &error));
> 
>   DEBUG ("Request ContactInfo cancelled");
> 
>   g_simple_async_result_set_from_error (data->result, error);
>-  g_simple_async_result_complete (data->result);
>+  g_simple_async_result_complete_in_idle (data->result);
>   g_clear_error (&error);
> 
>   g_assert (data->call != NULL);
>   tp_proxy_pending_call_cancel (data->call);
>-
>-  return FALSE;
>-}
>-
>-static void
>-contact_info_request_cancelled_cb (GCancellable *cancellable,
>-    ContactInfoRequestData *data)
>-{
>-  /* Cancellable is locked here, and g_cancellable_disconnect() also try to get
>-   * the lock. That could then make deadlock. To be safe we cancel the DBus call
>-   * in an idle callback. Use G_PRIORITY_HIGH to be sure our callback comes
>-   * before the DBus reply otherwise it could be racy. */
>-  data->cancel_idle_id = g_idle_add_full (G_PRIORITY_HIGH,
>-      contact_info_request_cancelled_idle_cb, data, NULL);
> }
> 
> /**
>@@ -2637,12 +2617,6 @@ tp_contact_request_contact_info_async (TpContact *self,
>   data->result = g_simple_async_result_new (G_OBJECT (self), callback,
>       user_data, tp_contact_request_contact_info_finish);
> 
>-  data->call = tp_cli_connection_interface_contact_info_call_request_contact_info (
>-      self->priv->connection, 60*60*1000, self->priv->handle,
>-      contact_info_request_cb,
>-      data, (GDestroyNotify) contact_info_request_data_free,
>-      NULL);
>-
>   if (cancellable != NULL)
>     {
>       data->cancellable = g_object_ref (cancellable);
>@@ -2650,6 +2624,11 @@ tp_contact_request_contact_info_async (TpContact *self,
>           G_CALLBACK (contact_info_request_cancelled_cb), data, NULL);
>     }
> 
>+  data->call = tp_cli_connection_interface_contact_info_call_request_contact_info (
>+      self->priv->connection, 60*60*1000, self->priv->handle,
>+      contact_info_request_cb,
>+      data, (GDestroyNotify) contact_info_request_data_free,
>+      NULL);
> }
> 
> /**
>-- 
>1.7.1
>

Actions: View | Splinter Review

Attachments on bug 28920: 36764 | 36783 | 36784

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.