Patch: prevent send queue to launch two sending threads

[Sergio Villar Senin <svillar igalia com>]

Hi,

this is a fix that we developed for the stable branch of tinymail and
that is worth merging to trunk. Basically the problem that we detected
is that create_worker could create two different threads to process
emails. The reason was that priv->is_running was not set until the
thread started to run, so it could happen that there were two calls to
create_worker that could start two different threads accessing the same
data without any locking to protect things like priv->cancel
CamelOperation for example.

BTW the attached patch includes also the patch I sent before in an email
with subject "Patch: change in send queue behaviour when notifying the
user" because it was not committed before, I completely forgot it :)

Basically the modifications for this issue are the ones related to
priv->is_running and the new priv->running_lock

Br

Index: libtinymail-camel/tny-camel-send-queue-priv.h
===================================================================
--- libtinymail-camel/tny-camel-send-queue-priv.h	(revision 3813)
+++ libtinymail-camel/tny-camel-send-queue-priv.h	(working copy)
@@ -36,6 +36,7 @@
 	gboolean do_continue, is_running;
 	gboolean observer_attached;
 	gint pending_send_notifies;
+	GStaticMutex *running_lock; 
 };
 
 #endif
Index: libtinymail-camel/tny-camel-send-queue.c
===================================================================
--- libtinymail-camel/tny-camel-send-queue.c	(revision 3813)
+++ libtinymail-camel/tny-camel-send-queue.c	(working copy)
@@ -68,6 +68,9 @@
 	TnyHeader *header;
 	gint i, total;
 	guint signal_id;
+	GCond *condition;
+	GMutex *mutex;
+	gboolean had_callback;
 } ControlInfo;
 
 
@@ -308,6 +311,13 @@
 	if (apriv)
 		tny_lockable_unlock (apriv->session->priv->ui_lock);
 
+	if (info->mutex) {
+		g_mutex_lock (info->mutex);
+		g_cond_broadcast (info->condition);
+		info->had_callback = TRUE;
+		g_mutex_unlock (info->mutex);
+	}
+
 	return FALSE;
 }
 
@@ -567,6 +577,32 @@
 	g_slice_free (SyncSync, info);
 }
 
+static void
+wait_for_queue_start_notification (TnySendQueue *self)
+{
+	ControlInfo *info = g_slice_new0 (ControlInfo);
+
+	info->mutex = g_mutex_new ();
+	info->condition = g_cond_new ();
+	info->had_callback = FALSE;
+
+	info->self = g_object_ref (self);
+	info->signal_id = TNY_SEND_QUEUE_START;
+
+	g_idle_add (emit_queue_control_signals_on_mainloop, info);
+
+	g_mutex_lock (info->mutex);
+	if (!info->had_callback)
+		g_cond_wait (info->condition, info->mutex);
+	g_mutex_unlock (info->mutex);
+
+	g_mutex_free (info->mutex);
+	g_cond_free (info->condition);
+
+	g_slice_free (GetHeadersSync, info);
+}
+
+
 static gpointer
 thread_main (gpointer data)
 {
@@ -578,7 +614,9 @@
 	TnyDevice *device = info->device;
 	GHashTable *failed_headers = NULL;
 
-	priv->is_running = TRUE;
+	/* Wait here until the user receives the queue-start notification */
+	wait_for_queue_start_notification (self);
+
 	list = tny_simple_list_new ();
 
 	g_static_rec_mutex_lock (priv->todo_lock);
@@ -621,7 +659,6 @@
 			if (tny_device_is_online (device)) 
 				get_headers_sync (info->outbox, headers, TRUE, &ferror);
 			else {
-				priv->is_running = FALSE;
 				g_static_rec_mutex_unlock (priv->todo_lock);
 				g_static_rec_mutex_unlock (priv->sending_lock);
 				goto errorhandler;
@@ -629,7 +666,6 @@
 
 			if (ferror != NULL)
 			{
-				priv->is_running = FALSE;
 				emit_error (self, NULL, NULL, ferror, i, priv->total);
 				g_error_free (ferror);
 				g_object_unref (headers);
@@ -676,7 +712,6 @@
 
 			if (length <= 0)
 			{
-				priv->is_running = FALSE;
 				g_object_unref (headers);
 				g_static_rec_mutex_unlock (priv->todo_lock);
 				g_static_rec_mutex_unlock (priv->sending_lock);
@@ -765,7 +800,6 @@
 			g_object_unref (header);
 		} else 
 		{
-			priv->is_running = FALSE;
 			/* Not good, or we just went offline, let's just kill this thread */ 
 			length = 0;
 			if (header && G_IS_OBJECT (header))
@@ -783,8 +817,6 @@
 
 errorhandler:
 
-	priv->is_running = FALSE;
-
 	if (failed_headers)
 		g_hash_table_destroy (failed_headers);
 
@@ -814,6 +846,10 @@
 
 	g_slice_free (MainThreadInfo, info);
 
+	g_static_mutex_lock (priv->running_lock);
+	priv->is_running = FALSE;
+	g_static_mutex_unlock (priv->running_lock);
+
 	/* Emit the queue-stop signal */
 	emit_queue_control_signals (self, TNY_SEND_QUEUE_STOP);
 
@@ -827,6 +863,7 @@
 {
 	TnyCamelSendQueuePriv *priv = TNY_CAMEL_SEND_QUEUE_GET_PRIVATE (self);
 
+	g_static_mutex_lock (priv->running_lock);
 	if (!priv->is_running && priv->trans_account && TNY_IS_TRANSPORT_ACCOUNT (priv->trans_account))
 	{
 		TnyCamelAccountPriv *apriv = TNY_CAMEL_ACCOUNT_GET_PRIVATE (priv->trans_account);
@@ -871,12 +908,19 @@
 				_tny_camel_folder_reason (spriv);
 			}
 
-			emit_queue_control_signals (self, TNY_SEND_QUEUE_START);
-
+			priv->is_running = TRUE;
 			priv->thread = g_thread_create (thread_main, info, FALSE, NULL);
 
 			if (priv->thread == NULL) {
-				emit_queue_control_signals (self, TNY_SEND_QUEUE_STOP);
+				GError *error;
+
+				priv->is_running = FALSE;
+
+				g_set_error (err, TNY_ERROR_DOMAIN, 
+					     TNY_SYSTEM_ERROR_UNKNOWN,
+					     "Couldn't start thread for send queue");
+				emit_error (self, NULL, NULL, error, 0, 0);
+
 			        if (TNY_IS_CAMEL_FOLDER (info->outbox)) {
 		        	        TnyCamelFolderPriv *opriv = TNY_CAMEL_FOLDER_GET_PRIVATE (info->outbox);
 					_tny_camel_folder_unreason (opriv);
@@ -888,6 +932,7 @@
 			}
 		}
 	}
+	g_static_mutex_unlock (priv->running_lock);
 
 	return;
 }
@@ -909,7 +954,9 @@
 
 	g_static_rec_mutex_lock (priv->sending_lock);
 
+	g_static_mutex_lock (priv->running_lock);
 	priv->is_running = FALSE;
+	g_static_mutex_unlock (priv->running_lock);
 
 	outbox = tny_send_queue_get_outbox (self);
 
@@ -991,7 +1038,7 @@
 	if (!err)
 		priv->total++;
 
-	if (!err & !priv->is_running)
+	if (!err)
 		create_worker (info->self, &new_err);
 
 	/* Call user callback after msg has beed added to OUTBOX, waiting to be sent*/
@@ -1270,6 +1317,9 @@
 	g_free (priv->sending_lock);
 	priv->sending_lock = NULL;
 
+	g_free (priv->running_lock);
+	priv->running_lock = NULL;
+
 	g_object_unref (priv->trans_account);
 
 	(*parent_class->finalize) (object);
@@ -1472,6 +1522,8 @@
 	g_static_rec_mutex_init (priv->todo_lock);
 	priv->sending_lock = g_new0 (GStaticRecMutex, 1);
 	g_static_rec_mutex_init (priv->sending_lock);
+	priv->running_lock = g_new0 (GStaticMutex, 1);
+	g_static_mutex_init (priv->running_lock);
 
 	priv->is_running = FALSE;
 	priv->thread = NULL;
Index: ChangeLog
===================================================================
--- ChangeLog	(revision 3813)
+++ ChangeLog	(working copy)
@@ -1,7 +1,11 @@
 2008-11-19  Sergio Villar Senin  <svillar igalia com>
 
+	* libtinymail-camel/tny-camel-send-queue-priv.h
+	* libtinymail-camel/tny-camel-send-queue.c: added some locking to
+	prevent two send queue threads running at the same time.
+
 	* libtinymail-camel/tny-camel-mime-part.c
-	(tny_camel_mime_part_decode_to_stream_default): 
+	(tny_camel_mime_part_decode_to_stream_default):
 	adds a reference to protect wrapper when decoding to a stream
 
 2008-11-19  Jose Dapena Paz  <jdapena igalia com>