GNOME.org
 

[libgdata] core: Ensure GDataBatchOperation's operations' callbacks are always called

commit 17142ca07f96d671ad07c8be48df5892bd8187c2
Author: Philip Withnall <philip tecnocode co uk>
Date:   Thu Dec 9 21:45:51 2010 +0000

    core: Ensure GDataBatchOperation's operations' callbacks are always called
    
    Even in error conditions, the callbacks for each of the operations in a
    GDataBatchOperation should be called, or memory leaks could result.

 gdata/gdata-batch-feed.c      |    6 +-
 gdata/gdata-batch-operation.c |   83 +++++++++++++++++++++++++++++++----------
 gdata/tests/common.c          |   59 +++++++++++++++++++++++++++++
 gdata/tests/common.h          |    3 +
 gdata/tests/documents.c       |   31 ++++++++++-----
 5 files changed, 149 insertions(+), 33 deletions(-)
---
diff --git a/gdata/gdata-batch-feed.c b/gdata/gdata-batch-feed.c
index 46df2cb..26e8320 100644
--- a/gdata/gdata-batch-feed.c
+++ b/gdata/gdata-batch-feed.c
@@ -145,10 +145,10 @@ parse_xml (GDataParsable *parsable, xmlDoc *doc, xmlNode *node, gpointer user_da
 		if (op->type != GDATA_BATCH_OPERATION_DELETION && entry == NULL)
 			goto error;
 
-		if (entry != NULL) {
-			_gdata_batch_operation_run_callback (operation, op, entry, NULL);
+		_gdata_batch_operation_run_callback (operation, op, entry, NULL);
+
+		if (entry != NULL)
 			g_object_unref (entry);
-		}
 
 		g_free (status_reason);
 		xmlBufferFree (status_response);
diff --git a/gdata/gdata-batch-operation.c b/gdata/gdata-batch-operation.c
index df3617a..9783b15 100644
--- a/gdata/gdata-batch-operation.c
+++ b/gdata/gdata-batch-operation.c
@@ -298,13 +298,16 @@ _gdata_batch_operation_get_operation (GDataBatchOperation *self, guint id)
 }
 
 /* Run a user-supplied callback for a #BatchOperation whose return value we've just processed. This is designed to be used in an idle handler, so
- * that the callback is run in the main thread. It should not be called if the user-supplied callback is %NULL. */
+ * that the callback is run in the main thread. It can be called if the user-supplied callback is %NULL (e.g. in the case that the callback's been
+ * called before). */
 static gboolean
 run_callback_cb (BatchOperation *op)
 {
-	/* We do allow op->callback to be unset, but in that case run_callback_cb() shouldn't be being called */
-	g_assert (op->callback != NULL);
-	op->callback (op->id, op->type, op->entry, op->error, op->user_data);
+	if (op->callback != NULL)
+		op->callback (op->id, op->type, op->entry, op->error, op->user_data);
+
+	/* Unset the callback so that it can't be called again */
+	op->callback = NULL;
 
 	return FALSE;
 }
@@ -549,6 +552,9 @@ run_cb (gpointer key, BatchOperation *op, GDataFeed *feed)
  * callbacks synchronously (i.e. before gdata_batch_operation_run() returns, and in the same thread that called gdata_batch_operation_run()) as the
  * server returns results for each operation.
  *
+ * The callbacks for all of the operations in the batch operation are always guaranteed to be called, even if the batch operation as a whole fails.
+ * Each callback will be called exactly once for each time gdata_batch_operation_run() is called.
+ *
  * The return value of the function indicates whether the overall batch operation was successful, and doesn't indicate the status of any of the
  * operations it comprises. gdata_batch_operation_run() could return %TRUE even if all of its operations failed.
  *
@@ -568,6 +574,10 @@ gdata_batch_operation_run (GDataBatchOperation *self, GCancellable *cancellable,
 	GTimeVal updated;
 	gchar *upload_data;
 	guint status;
+	GHashTableIter iter;
+	gpointer op_id;
+	BatchOperation *op;
+	GError *child_error = NULL;
 
 	g_return_val_if_fail (GDATA_IS_BATCH_OPERATION (self), FALSE);
 	g_return_val_if_fail (cancellable == NULL || G_IS_CANCELLABLE (cancellable), FALSE);
@@ -590,33 +600,43 @@ gdata_batch_operation_run (GDataBatchOperation *self, GCancellable *cancellable,
 	priv->has_run = TRUE;
 
 	/* Send the message */
-	status = _gdata_service_send_message (priv->service, message, cancellable, error);
-
-	if (status == SOUP_STATUS_NONE || status == SOUP_STATUS_CANCELLED) {
-		/* Redirect error or cancelled */
-		g_object_unref (message);
-		return FALSE;
-	} else if (status != SOUP_STATUS_OK) {
-		/* Error */
-		GDataServiceClass *klass = GDATA_SERVICE_GET_CLASS (priv->service);
-		g_assert (klass->parse_error_response != NULL);
-		klass->parse_error_response (priv->service, GDATA_OPERATION_BATCH, status, message->reason_phrase, message->response_body->data,
-		                             message->response_body->length, error);
+	status = _gdata_service_send_message (priv->service, message, cancellable, &child_error);
+
+	if (status != SOUP_STATUS_OK) {
+		/* Iff status is SOUP_STATUS_NONE or SOUP_STATUS_CANCELLED, child_error has already been set */
+		if (status != SOUP_STATUS_NONE && status != SOUP_STATUS_CANCELLED) {
+			/* Error */
+			GDataServiceClass *klass = GDATA_SERVICE_GET_CLASS (priv->service);
+			g_assert (klass->parse_error_response != NULL);
+			klass->parse_error_response (priv->service, GDATA_OPERATION_BATCH, status, message->reason_phrase, message->response_body->data,
+			                             message->response_body->length, &child_error);
+		}
 		g_object_unref (message);
-		return FALSE;
+
+		goto error;
 	}
 
 	/* Parse the XML; GDataBatchFeed will fire off the relevant callbacks */
 	g_assert (message->response_body->data != NULL);
 	feed = GDATA_FEED (_gdata_parsable_new_from_xml (GDATA_TYPE_BATCH_FEED, message->response_body->data, message->response_body->length,
-	                                                 self, error));
+	                                                 self, &child_error));
 	g_object_unref (message);
 
 	if (feed == NULL)
-		return FALSE;
+		goto error;
 	g_object_unref (feed);
 
 	return TRUE;
+
+error:
+	/* Call the callbacks for each of our operations to notify them of the error */
+	g_hash_table_iter_init (&iter, priv->operations);
+	while (g_hash_table_iter_next (&iter, &op_id, (gpointer*) &op) == TRUE)
+		_gdata_batch_operation_run_callback (self, op, NULL, g_error_copy (child_error));
+
+	g_propagate_error (error, child_error);
+
+	return FALSE;
 }
 
 static void
@@ -688,6 +708,8 @@ gdata_batch_operation_run_async (GDataBatchOperation *self, GCancellable *cancel
 gboolean
 gdata_batch_operation_run_finish (GDataBatchOperation *self, GAsyncResult *async_result, GError **error)
 {
+	GDataBatchOperationPrivate *priv = self->priv;
+	GError *child_error = NULL;
 	GSimpleAsyncResult *result = G_SIMPLE_ASYNC_RESULT (async_result);
 
 	g_return_val_if_fail (GDATA_IS_BATCH_OPERATION (self), FALSE);
@@ -696,8 +718,29 @@ gdata_batch_operation_run_finish (GDataBatchOperation *self, GAsyncResult *async
 
 	g_warn_if_fail (g_simple_async_result_get_source_tag (result) == gdata_batch_operation_run_async);
 
-	if (g_simple_async_result_propagate_error (result, error) == TRUE)
+	if (g_simple_async_result_propagate_error (result, &child_error) == TRUE) {
+		if (priv->has_run == FALSE) {
+			GHashTableIter iter;
+			gpointer op_id;
+			BatchOperation *op;
+
+			/* Temporarily mark the operation as synchronous so that the callbacks get dispatched in this thread */
+			priv->is_async = FALSE;
+
+			/* If has_run hasn't been set, the call to gdata_batch_operation_run() was never made in the thread, and so none of the
+			 * operations' callbacks have been called. Call the callbacks for each of our operations to notify them of the error.
+			 * If has_run has been set, gdata_batch_operation_run() has already done this for us. */
+			g_hash_table_iter_init (&iter, priv->operations);
+			while (g_hash_table_iter_next (&iter, &op_id, (gpointer*) &op) == TRUE)
+				_gdata_batch_operation_run_callback (self, op, NULL, g_error_copy (child_error));
+
+			priv->is_async = TRUE;
+		}
+
+		g_propagate_error (error, child_error);
+
 		return FALSE;
+	}
 
 	return g_simple_async_result_get_op_res_gboolean (result);
 }
diff --git a/gdata/tests/common.c b/gdata/tests/common.c
index 37d3e7d..7391957 100644
--- a/gdata/tests/common.c
+++ b/gdata/tests/common.c
@@ -84,6 +84,7 @@ gdata_test_internet (void)
 }
 
 typedef struct {
+	GDataBatchOperation *operation;
 	guint op_id;
 	GDataBatchOperationType operation_type;
 	GDataEntry *entry;
@@ -96,6 +97,8 @@ typedef struct {
 static void
 batch_operation_data_free (BatchOperationData *data)
 {
+	if (data->operation != NULL)
+		g_object_unref (data->operation);
 	if (data->entry != NULL)
 		g_object_unref (data->entry);
 	g_free (data->id);
@@ -110,6 +113,10 @@ test_batch_operation_query_cb (guint operation_id, GDataBatchOperationType opera
 {
 	BatchOperationData *data = user_data;
 
+	/* Mark the callback as having been run */
+	g_object_set_data (G_OBJECT (data->operation), "test::called-callbacks",
+	                   GUINT_TO_POINTER (GPOINTER_TO_UINT (g_object_get_data (G_OBJECT (data->operation), "test::called-callbacks")) + 1));
+
 	/* Check that the @operation_type and @operation_id matches those stored in @data */
 	g_assert_cmpuint (operation_id, ==, data->op_id);
 	g_assert_cmpuint (operation_type, ==, data->operation_type);
@@ -159,6 +166,7 @@ gdata_test_batch_operation_query (GDataBatchOperation *operation, const gchar *i
 	BatchOperationData *data;
 
 	data = g_slice_new (BatchOperationData);
+	data->operation = g_object_ref (operation);
 	data->op_id = 0;
 	data->operation_type = GDATA_BATCH_OPERATION_QUERY;
 	data->entry = g_object_ref (entry);
@@ -171,6 +179,10 @@ gdata_test_batch_operation_query (GDataBatchOperation *operation, const gchar *i
 
 	data->op_id = op_id;
 
+	/* We expect a callback to be called when the operation is run */
+	g_object_set_data (G_OBJECT (operation), "test::expected-callbacks",
+	                   GUINT_TO_POINTER (GPOINTER_TO_UINT (g_object_get_data (G_OBJECT (operation), "test::expected-callbacks")) + 1));
+
 	return op_id;
 }
 
@@ -180,6 +192,10 @@ test_batch_operation_insertion_update_cb (guint operation_id, GDataBatchOperatio
 {
 	BatchOperationData *data = user_data;
 
+	/* Mark the callback as having been run */
+	g_object_set_data (G_OBJECT (data->operation), "test::called-callbacks",
+	                   GUINT_TO_POINTER (GPOINTER_TO_UINT (g_object_get_data (G_OBJECT (data->operation), "test::called-callbacks")) + 1));
+
 	/* Check that the @operation_type and @operation_id matches those stored in @data */
 	g_assert_cmpuint (operation_id, ==, data->op_id);
 	g_assert_cmpuint (operation_type, ==, data->operation_type);
@@ -225,6 +241,7 @@ gdata_test_batch_operation_insertion (GDataBatchOperation *operation, GDataEntry
 	BatchOperationData *data;
 
 	data = g_slice_new (BatchOperationData);
+	data->operation = g_object_ref (operation);
 	data->op_id = 0;
 	data->operation_type = GDATA_BATCH_OPERATION_INSERTION;
 	data->entry = g_object_ref (entry);
@@ -237,6 +254,10 @@ gdata_test_batch_operation_insertion (GDataBatchOperation *operation, GDataEntry
 
 	data->op_id = op_id;
 
+	/* We expect a callback to be called when the operation is run */
+	g_object_set_data (G_OBJECT (operation), "test::expected-callbacks",
+	                   GUINT_TO_POINTER (GPOINTER_TO_UINT (g_object_get_data (G_OBJECT (operation), "test::expected-callbacks")) + 1));
+
 	return op_id;
 }
 
@@ -247,6 +268,7 @@ gdata_test_batch_operation_update (GDataBatchOperation *operation, GDataEntry *e
 	BatchOperationData *data;
 
 	data = g_slice_new (BatchOperationData);
+	data->operation = g_object_ref (operation);
 	data->op_id = 0;
 	data->operation_type = GDATA_BATCH_OPERATION_UPDATE;
 	data->entry = g_object_ref (entry);
@@ -259,6 +281,10 @@ gdata_test_batch_operation_update (GDataBatchOperation *operation, GDataEntry *e
 
 	data->op_id = op_id;
 
+	/* We expect a callback to be called when the operation is run */
+	g_object_set_data (G_OBJECT (operation), "test::expected-callbacks",
+	                   GUINT_TO_POINTER (GPOINTER_TO_UINT (g_object_get_data (G_OBJECT (operation), "test::expected-callbacks")) + 1));
+
 	return op_id;
 }
 
@@ -267,6 +293,10 @@ test_batch_operation_deletion_cb (guint operation_id, GDataBatchOperationType op
 {
 	BatchOperationData *data = user_data;
 
+	/* Mark the callback as having been run */
+	g_object_set_data (G_OBJECT (data->operation), "test::called-callbacks",
+	                   GUINT_TO_POINTER (GPOINTER_TO_UINT (g_object_get_data (G_OBJECT (data->operation), "test::called-callbacks")) + 1));
+
 	/* Check that the @operation_type and @operation_id matches those stored in @data */
 	g_assert_cmpuint (operation_id, ==, data->op_id);
 	g_assert_cmpuint (operation_type, ==, data->operation_type);
@@ -291,6 +321,7 @@ gdata_test_batch_operation_deletion (GDataBatchOperation *operation, GDataEntry
 	BatchOperationData *data;
 
 	data = g_slice_new (BatchOperationData);
+	data->operation = g_object_ref (operation);
 	data->op_id = 0;
 	data->operation_type = GDATA_BATCH_OPERATION_DELETION;
 	data->entry = g_object_ref (entry);
@@ -303,5 +334,33 @@ gdata_test_batch_operation_deletion (GDataBatchOperation *operation, GDataEntry
 
 	data->op_id = op_id;
 
+	/* We expect a callback to be called when the operation is run */
+	g_object_set_data (G_OBJECT (operation), "test::expected-callbacks",
+	                   GUINT_TO_POINTER (GPOINTER_TO_UINT (g_object_get_data (G_OBJECT (operation), "test::expected-callbacks")) + 1));
+
 	return op_id;
 }
+
+gboolean
+gdata_test_batch_operation_run (GDataBatchOperation *operation, GCancellable *cancellable, GError **error)
+{
+	gboolean success = gdata_batch_operation_run (operation, cancellable, error);
+
+	/* Assert that callbacks were called exactly once for each operation in the batch operation */
+	g_assert_cmpuint (GPOINTER_TO_UINT (g_object_get_data (G_OBJECT (operation), "test::expected-callbacks")), ==,
+	                  GPOINTER_TO_UINT (g_object_get_data (G_OBJECT (operation), "test::called-callbacks")));
+
+	return success;
+}
+
+gboolean
+gdata_test_batch_operation_run_finish (GDataBatchOperation *operation, GAsyncResult *async_result, GError **error)
+{
+	gboolean success = gdata_batch_operation_run_finish (operation, async_result, error);
+
+	/* Assert that callbacks were called exactly once for each operation in the batch operation */
+	g_assert_cmpuint (GPOINTER_TO_UINT (g_object_get_data (G_OBJECT (operation), "test::expected-callbacks")), ==,
+	                  GPOINTER_TO_UINT (g_object_get_data (G_OBJECT (operation), "test::called-callbacks")));
+
+	return success;
+}
diff --git a/gdata/tests/common.h b/gdata/tests/common.h
index 4c9492d..74746ee 100644
--- a/gdata/tests/common.h
+++ b/gdata/tests/common.h
@@ -40,6 +40,9 @@ guint gdata_test_batch_operation_insertion (GDataBatchOperation *operation, GDat
 guint gdata_test_batch_operation_update (GDataBatchOperation *operation, GDataEntry *entry, GDataEntry **updated_entry, GError **error);
 guint gdata_test_batch_operation_deletion (GDataBatchOperation *operation, GDataEntry *entry, GError **error);
 
+gboolean gdata_test_batch_operation_run (GDataBatchOperation *operation, GCancellable *cancellable, GError **error);
+gboolean gdata_test_batch_operation_run_finish (GDataBatchOperation *operation, GAsyncResult *async_result, GError **error);
+
 G_END_DECLS
 
 #endif /* !GDATA_TEST_COMMON_H */
diff --git a/gdata/tests/documents.c b/gdata/tests/documents.c
index fc1ee0b..0995978 100644
--- a/gdata/tests/documents.c
+++ b/gdata/tests/documents.c
@@ -1125,7 +1125,7 @@ test_batch (gconstpointer service)
 	gdata_entry_set_title (GDATA_ENTRY (doc), "My First Document");
 
 	gdata_test_batch_operation_insertion (operation, GDATA_ENTRY (doc), &inserted_entry, NULL);
-	g_assert (gdata_batch_operation_run (operation, NULL, &error) == TRUE);
+	g_assert (gdata_test_batch_operation_run (operation, NULL, &error) == TRUE);
 	g_assert_no_error (error);
 
 	g_clear_error (&error);
@@ -1142,7 +1142,7 @@ test_batch (gconstpointer service)
 	                                           NULL);
 	g_assert_cmpuint (op_id, !=, op_id2);
 
-	g_assert (gdata_batch_operation_run (operation, NULL, &error) == TRUE);
+	g_assert (gdata_test_batch_operation_run (operation, NULL, &error) == TRUE);
 	g_assert_no_error (error);
 
 	g_clear_error (&error);
@@ -1155,7 +1155,7 @@ test_batch (gconstpointer service)
 	gdata_test_batch_operation_query (operation, gdata_entry_get_id (inserted_entry), GDATA_TYPE_DOCUMENTS_TEXT, inserted_entry,
 	                                  &inserted_entry_updated, NULL);
 
-	g_assert (gdata_batch_operation_run (operation, NULL, &error) == TRUE);
+	g_assert (gdata_test_batch_operation_run (operation, NULL, &error) == TRUE);
 	g_assert_no_error (error);
 
 	g_clear_error (&error);
@@ -1169,7 +1169,7 @@ test_batch (gconstpointer service)
 	gdata_test_batch_operation_query (operation, gdata_entry_get_id (inserted_entry2), GDATA_TYPE_DOCUMENTS_TEXT, inserted_entry2,
 	                                  &inserted_entry2_updated, NULL);
 
-	g_assert (gdata_batch_operation_run (operation, NULL, &error) == TRUE);
+	g_assert (gdata_test_batch_operation_run (operation, NULL, &error) == TRUE);
 	g_assert_no_error (error);
 
 	g_clear_error (&error);
@@ -1187,7 +1187,7 @@ test_batch (gconstpointer service)
 	g_assert_cmpuint (op_id, !=, op_id3);
 	g_assert_cmpuint (op_id2, !=, op_id3);
 
-	g_assert (gdata_batch_operation_run (operation, NULL, &error) == TRUE);
+	g_assert (gdata_test_batch_operation_run (operation, NULL, &error) == TRUE);
 	g_assert_no_error (error);
 
 	g_assert_error (entry_error, GDATA_SERVICE_ERROR, GDATA_SERVICE_ERROR_PROTOCOL_ERROR);
@@ -1203,7 +1203,7 @@ test_batch (gconstpointer service)
 	 * to test error handling */
 	operation = gdata_batchable_create_operation (GDATA_BATCHABLE (service), "https://docs.google.com/feeds/documents/private/full/batch";);
 	gdata_test_batch_operation_update (operation, inserted_entry2, NULL, &entry_error);
-	g_assert (gdata_batch_operation_run (operation, NULL, &error) == TRUE);
+	g_assert (gdata_test_batch_operation_run (operation, NULL, &error) == TRUE);
 	g_assert_no_error (error);
 
 	g_assert_error (entry_error, GDATA_SERVICE_ERROR, GDATA_SERVICE_ERROR_CONFLICT);
@@ -1216,7 +1216,7 @@ test_batch (gconstpointer service)
 	/* Run a final batch operation to delete the second entry */
 	operation = gdata_batchable_create_operation (GDATA_BATCHABLE (service), "https://docs.google.com/feeds/documents/private/full/batch";);
 	gdata_test_batch_operation_deletion (operation, inserted_entry3, NULL);
-	g_assert (gdata_batch_operation_run (operation, NULL, &error) == TRUE);
+	g_assert (gdata_test_batch_operation_run (operation, NULL, &error) == TRUE);
 	g_assert_no_error (error);
 
 	g_clear_error (&error);
@@ -1255,7 +1255,10 @@ test_batch_async_cb (GDataBatchOperation *operation, GAsyncResult *async_result,
 {
 	GError *error = NULL;
 
-	g_assert (gdata_batch_operation_run_finish (operation, async_result, &error) == TRUE);
+	/* Clear all pending events (such as callbacks for the operations) */
+	while (g_main_context_iteration (NULL, FALSE) == TRUE);
+
+	g_assert (gdata_test_batch_operation_run_finish (operation, async_result, &error) == TRUE);
 	g_assert_no_error (error);
 	g_clear_error (&error);
 
@@ -1288,7 +1291,10 @@ test_batch_async_cancellation_cb (GDataBatchOperation *operation, GAsyncResult *
 {
 	GError *error = NULL;
 
-	g_assert (gdata_batch_operation_run_finish (operation, async_result, &error) == FALSE);
+	/* Clear all pending events (such as callbacks for the operations) */
+	while (g_main_context_iteration (NULL, FALSE) == TRUE);
+
+	g_assert (gdata_test_batch_operation_run_finish (operation, async_result, &error) == FALSE);
 	g_assert_error (error, G_IO_ERROR, G_IO_ERROR_CANCELLED);
 	g_clear_error (&error);
 
@@ -1302,11 +1308,12 @@ test_batch_async_cancellation (BatchAsyncData *data, gconstpointer service)
 	guint op_id;
 	GMainLoop *main_loop;
 	GCancellable *cancellable;
+	GError *error = NULL;
 
 	/* Run an async query operation on the document */
 	operation = gdata_batchable_create_operation (GDATA_BATCHABLE (service), "https://docs.google.com/feeds/documents/private/full/batch";);
 	op_id = gdata_test_batch_operation_query (operation, gdata_entry_get_id (GDATA_ENTRY (data->new_doc)), GDATA_TYPE_DOCUMENTS_TEXT,
-	                                          GDATA_ENTRY (data->new_doc), NULL, NULL);
+	                                          GDATA_ENTRY (data->new_doc), NULL, &error);
 
 	main_loop = g_main_loop_new (NULL, TRUE);
 	cancellable = g_cancellable_new ();
@@ -1315,6 +1322,10 @@ test_batch_async_cancellation (BatchAsyncData *data, gconstpointer service)
 	g_cancellable_cancel (cancellable); /* this should cancel the operation before it even starts, as we haven't run the main loop yet */
 
 	g_main_loop_run (main_loop);
+
+	g_assert_error (error, G_IO_ERROR, G_IO_ERROR_CANCELLED);
+	g_clear_error (&error);
+
 	g_main_loop_unref (main_loop);
 	g_object_unref (cancellable);
 	g_object_unref (operation);
[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]