Patch: sort locks on imap idle_thread to avoid deadlocks

[José Dapena Paz <jdapena igalia com>]

	Hi,

	This patch should solve a longstanding issue in the idle_thread, where
we were suffering deadlocks on joinining the idle thread in
camel_imap_folder_stop_idle.

	The ideas behind the change:
	* Sort locks: first service connect_lock, then idle_t_lock.
	* Protect all the idle_thread operations inside the loop with these
locks. This way, joining has to wait until we finish an iteration in
idle thread.
	* Make idle_kill be true always when we join to make sure that
idle_thread loop doesn't try to enter again in the operations.
	* In idle_thread, the locks are obtained using trylock, to avoid
waiting for the locks that join thread could be waiting for.

	We have been testing the patch last days, and it seems it finally gets
rid of the "not-ui hang" where sometimes retrieving progressbar lasted
forever in modest.

-- 
José Dapena Paz <jdapena igalia com>
Igalia

Index: m4/gtk-doc.m4
===================================================================
--- m4/gtk-doc.m4	(revision 3778)
+++ m4/gtk-doc.m4	(working copy)
@@ -9,31 +9,45 @@
   AC_BEFORE([AC_PROG_LIBTOOL],[$0])dnl setup libtool first
   AC_BEFORE([AM_PROG_LIBTOOL],[$0])dnl setup libtool first
   dnl for overriding the documentation installation directory
-  AC_ARG_WITH([html-dir],
-    AS_HELP_STRING([--with-html-dir=PATH], [path to installed docs]),,
+  AC_ARG_WITH(html-dir,
+    AC_HELP_STRING([--with-html-dir=PATH], [path to installed docs]),,
     [with_html_dir='${datadir}/gtk-doc/html'])
   HTML_DIR="$with_html_dir"
-  AC_SUBST([HTML_DIR])
+  AC_SUBST(HTML_DIR)
 
   dnl enable/disable documentation building
-  AC_ARG_ENABLE([gtk-doc],
-    AS_HELP_STRING([--enable-gtk-doc],
-                   [use gtk-doc to build documentation [[default=no]]]),,
-    [enable_gtk_doc=no])
+  AC_ARG_ENABLE(gtk-doc,
+    AC_HELP_STRING([--enable-gtk-doc],
+                   [use gtk-doc to build documentation [default=no]]),,
+    enable_gtk_doc=no)
 
+  have_gtk_doc=no
   if test x$enable_gtk_doc = xyes; then
-    ifelse([$1],[],
-      [PKG_CHECK_EXISTS([gtk-doc],,
-                        AC_MSG_ERROR([gtk-doc not installed and --enable-gtk-doc requested]))],
-      [PKG_CHECK_EXISTS([gtk-doc >= $1],,
-                        AC_MSG_ERROR([You need to have gtk-doc >= $1 installed to build gtk-doc]))])
+    if test -z "$PKG_CONFIG"; then
+      AC_PATH_PROG(PKG_CONFIG, pkg-config, no)
+    fi
+    if test "$PKG_CONFIG" != "no" && $PKG_CONFIG --exists gtk-doc; then
+      have_gtk_doc=yes
+    fi
+
+  dnl do we want to do a version check?
+ifelse([$1],[],,
+    [gtk_doc_min_version=$1
+    if test "$have_gtk_doc" = yes; then
+      AC_MSG_CHECKING([gtk-doc version >= $gtk_doc_min_version])
+      if $PKG_CONFIG --atleast-version $gtk_doc_min_version gtk-doc; then
+        AC_MSG_RESULT(yes)
+      else
+        AC_MSG_RESULT(no)
+        have_gtk_doc=no
+      fi
+    fi
+])
+    if test "$have_gtk_doc" != yes; then
+      enable_gtk_doc=no
+    fi
   fi
 
-  AC_MSG_CHECKING([whether to build gtk-doc documentation])
-  AC_MSG_RESULT($enable_gtk_doc)
-
-  AC_PATH_PROGS(GTKDOC_CHECK,gtkdoc-check,)
-
-  AM_CONDITIONAL([ENABLE_GTK_DOC], [test x$enable_gtk_doc = xyes])
-  AM_CONDITIONAL([GTK_DOC_USE_LIBTOOL], [test -n "$LIBTOOL"])
+  AM_CONDITIONAL(ENABLE_GTK_DOC, test x$enable_gtk_doc = xyes)
+  AM_CONDITIONAL(GTK_DOC_USE_LIBTOOL, test -n "$LIBTOOL")
 ])
Index: libtinymail-camel/camel-lite/camel/providers/imap/camel-imap-folder.c
===================================================================
--- libtinymail-camel/camel-lite/camel/providers/imap/camel-imap-folder.c	(revision 3778)
+++ libtinymail-camel/camel-lite/camel/providers/imap/camel-imap-folder.c	(working copy)
@@ -4032,31 +4032,12 @@
 	/* While nothing has stopped us yet ...
 	 * TNY TODO: it would be nicer to use select() here, rather than usleep() */
 
-	while (my_cont && !store->idle_kill)
-	{
+	while (my_cont && !store->idle_kill) {
 		CamelException ex = CAMEL_EXCEPTION_INITIALISER;
 		char *resp = NULL;
 		IdleResponse *idle_resp = NULL;
 		gboolean senddone = FALSE;
 
-		/* A) The first time we will start the IDLE by sending IDLE to
-		 * the server and reading away the + continuation (check out the
-		 * idle_real_start function). We don't call this in the other
-		 * loop cycles of this while. */
-
-		if (store->idle_cont && first) {
-			gboolean l = g_static_rec_mutex_trylock (store->idle_lock);
-			if (!store->idle_kill)
-				idle_real_start (store);
-			if (l)
-				g_static_rec_mutex_unlock (store->idle_lock);
-			first = FALSE;
-		}
-
-		/* And we also send the broadcast to the caller of this thread:
-		 * We're started, and we're fine. It can continue. We don't call
-		 * this in the other loop cycles of this while. */
-
 		if (tfirst) {
 			if (info->condition) {
 				g_mutex_lock (info->mutex);
@@ -4067,163 +4048,179 @@
 			tfirst = FALSE;
 		}
 
-		if (g_static_rec_mutex_trylock (store->idle_lock))
-		{
-			/* B) This happens during the IDLE's body (after IDLE is
-			 * started and before DONE is sent). We read away the
-			 * lines in a non-blocking way. As soon as we have a
-			 * full line, that starts with '*', we consume it. */
-
-			if (!store->idle_kill) {
-				while (camel_imap_store_readline_nb (store, &resp, &ex) > 0)
-				{
-					if (resp && strlen (resp) > 1 && resp[0] == '*') {
-						if (!idle_resp)
-							idle_resp = idle_response_new (folder);
-						consume_idle_line (store, folder, resp, idle_resp);
+		if (CAMEL_SERVICE_REC_TRYLOCK (store, connect_lock)) {
+			if (g_static_rec_mutex_trylock (store->idle_t_lock)) {
+				
+				/* A) The first time we will start the IDLE by sending IDLE to
+				 * the server and reading away the + continuation (check out the
+				 * idle_real_start function). We don't call this in the other
+				 * loop cycles of this while. */
+				
+				if (store->idle_cont && first) {
+					gboolean l = g_static_rec_mutex_trylock (store->idle_lock);
+					if (!store->idle_kill)
+						idle_real_start (store);
+					if (l)
+						g_static_rec_mutex_unlock (store->idle_lock);
+					first = FALSE;
+				}
+				
+				/* And we also send the broadcast to the caller of this thread:
+				 * We're started, and we're fine. It can continue. We don't call
+				 * this in the other loop cycles of this while. */
+				
+				if (g_static_rec_mutex_trylock (store->idle_lock)) {
+					/* B) This happens during the IDLE's body (after IDLE is
+					 * started and before DONE is sent). We read away the
+					 * lines in a non-blocking way. As soon as we have a
+					 * full line, that starts with '*', we consume it. */
+					
+					if (!store->idle_kill) {
+						while (camel_imap_store_readline_nb (store, &resp, &ex) > 0)
+							{
+								if (resp && strlen (resp) > 1 && resp[0] == '*') {
+									if (!idle_resp)
+										idle_resp = idle_response_new (folder);
+									consume_idle_line (store, folder, resp, idle_resp);
+								}
+								
+								if (resp)
+									g_free (resp);
+								resp = NULL;
+							}
 					}
-
-					if (resp)
-						g_free (resp);
-					resp = NULL;
+					g_static_rec_mutex_unlock (store->idle_lock);
 				}
-			}
-			g_static_rec_mutex_unlock (store->idle_lock);
-		}
-
-		if (resp)
-			g_free (resp);
-
-		if (store->idle_cont)
-		{
-			if (idle_resp && !idle_resp->exists_happened) {
-				/* We can process it already: nothing is at this moment
-				 * joining us, nothing is at this moment locking the
-				 * folder_changed handler of TnyCamelFolder */
-				process_idle_response (idle_resp);
-				idle_response_free (idle_resp);
-				idle_resp = NULL;
-				retval = NULL;
-			} else if (idle_resp && idle_resp->exists_happened) {
-
-				/* We can't deal with new EXISTS responses
-				 * without first stopping IDLE (we'll need to
-				 * fetch the new headers) */
-
-				senddone = TRUE;
-				retval = idle_resp;
-			}
-		} else {
-			/* If store->idle_cont was FALSE, we're going to handle
-			 * idle_resp differently (look below). */
-			senddone = TRUE;
-			retval = idle_resp;
-		}
-
-		/* C) So either we timed out (store->idle_sleep as been reached),
-		 * which means that we 'are' going to restart this entire while,
-		 * including resending the IDLE-start, after we're done with
-		 * this if-block of course.
-		 *
-		 * Or another thread called us to stop IDLE, and then we're
-		 * going to exit this while, of course. If it was an idle_kill,
-		 * we're not even going to try doing that in a nice way. In that
-		 * case we'll just exit ASAP (it's let_idle_die in CamelImapStore
-		 * trying to disconnect from the IMAP server). */
-
-		if ((cnt > store->idle_sleep) || senddone)
-		{
-			if (store->idle_prefix)
-			{
-				CamelImapResponseType type;
-				gboolean l = g_static_rec_mutex_trylock (store->idle_lock);
-
-				if (!store->idle_kill)
-				{
-					/* We read-away everything that we still
-					 * have. To find where idle_resp is handled,
-					 * read below at the g_thread_join for
-					 * this thread (we are returning it). */
-
-					resp = NULL;
-					while (camel_imap_store_readline_nb (store, &resp, &ex) > 0)
-					{
-						if (resp && strlen (resp) > 1 && resp[0] == '*') {
-							if (!idle_resp) {
-								idle_resp = idle_response_new (folder);
-								/* We will free this after the join */
-								if (!store->idle_cont)
-									retval = idle_resp;
+				
+				if (resp)
+					g_free (resp);
+				
+				if (store->idle_cont) {
+					if (idle_resp && !idle_resp->exists_happened) {
+						/* We can process it already: nothing is at this moment
+						 * joining us, nothing is at this moment locking the
+						 * folder_changed handler of TnyCamelFolder */
+						process_idle_response (idle_resp);
+						idle_response_free (idle_resp);
+						idle_resp = NULL;
+						retval = NULL;
+					} else if (idle_resp && idle_resp->exists_happened) {
+						
+						/* We can't deal with new EXISTS responses
+						 * without first stopping IDLE (we'll need to
+						 * fetch the new headers) */
+						
+						senddone = TRUE;
+						retval = idle_resp;
+					}
+				} else {
+					/* If store->idle_cont was FALSE, we're going to handle
+					 * idle_resp differently (look below). */
+					senddone = TRUE;
+					retval = idle_resp;
+				}
+				
+				/* C) So either we timed out (store->idle_sleep as been reached),
+				 * which means that we 'are' going to restart this entire while,
+				 * including resending the IDLE-start, after we're done with
+				 * this if-block of course.
+				 *
+				 * Or another thread called us to stop IDLE, and then we're
+				 * going to exit this while, of course. If it was an idle_kill,
+				 * we're not even going to try doing that in a nice way. In that
+				 * case we'll just exit ASAP (it's let_idle_die in CamelImapStore
+				 * trying to disconnect from the IMAP server). */
+				if ((cnt > store->idle_sleep) || senddone) {
+					if (store->idle_prefix) {
+						CamelImapResponseType type;
+						gboolean l = g_static_rec_mutex_trylock (store->idle_lock);
+						
+						if (!store->idle_kill) {
+							/* We read-away everything that we still
+							 * have. To find where idle_resp is handled,
+							 * read below at the g_thread_join for
+							 * this thread (we are returning it). */
+							
+							resp = NULL;
+							while (camel_imap_store_readline_nb (store, &resp, &ex) > 0) {
+								if (resp && strlen (resp) > 1 && resp[0] == '*') {
+									if (!idle_resp) {
+										idle_resp = idle_response_new (folder);
+										/* We will free this after the join */
+										if (!store->idle_cont)
+											retval = idle_resp;
+									}
+									consume_idle_line (store, folder, resp, idle_resp);
+								}
+								
+								if (resp)
+								g_free (resp);
+								resp = NULL;
 							}
-							consume_idle_line (store, folder, resp, idle_resp);
+							if (resp)
+								g_free (resp);
+							resp = NULL;
+							
+							/* We send the DONE to the server */
+							
+							nwritten = camel_stream_printf (store->ostream, "DONE\r\n");
+							idle_debug ("(%d, 8) -> DONE\n", nwritten);
+							
+							/* We read away everything the server sends
+							 * until the we see the untagged OK response */
+							
+							while ((type = camel_imap_command_response_idle (store, &resp, &ex)) == CAMEL_IMAP_RESPONSE_UNTAGGED) {
+								if (resp && strlen (resp) > 1 && resp[0] == '*') {
+									if (!idle_resp) {
+										idle_resp = idle_response_new (folder);
+										/* We will free this after the join */
+										if (!store->idle_cont)
+											retval = idle_resp;
+									}
+									consume_idle_line (store, folder, resp, idle_resp);
+								}
+								
+								if (resp)
+									g_free (resp);
+								resp = NULL;
+							}
 						}
-
+						
+						if (l)
+							g_static_rec_mutex_unlock (store->idle_lock);
+						
 						if (resp)
 							g_free (resp);
 						resp = NULL;
-					}
-					if (resp)
-						g_free (resp);
-					resp = NULL;
-
-					/* We send the DONE to the server */
-
-					nwritten = camel_stream_printf (store->ostream, "DONE\r\n");
-					idle_debug ("(%d, 8) -> DONE\n", nwritten);
-
-					/* We read away everything the server sends
-					 * until the we see the untagged OK response */
-
-					while ((type = camel_imap_command_response_idle (store, &resp, &ex)) == CAMEL_IMAP_RESPONSE_UNTAGGED)
-					{
-						if (resp && strlen (resp) > 1 && resp[0] == '*') {
-							if (!idle_resp) {
-								idle_resp = idle_response_new (folder);
-								/* We will free this after the join */
-								if (!store->idle_cont)
-									retval = idle_resp;
-							}
-							consume_idle_line (store, folder, resp, idle_resp);
+						
+						/* If we are continuing the loop, handle idle_resp
+						 * now (this can invoke fetching new headers). */
+						
+						if (store->idle_cont && idle_resp) {
+							process_idle_response (idle_resp);
+							idle_response_free (idle_resp);
+							idle_resp = NULL;
+							retval = NULL;
 						}
-
-						if (resp)
-							g_free (resp);
-						resp = NULL;
 					}
+					
+					if (store->idle_cont)
+						first = TRUE;
+					else
+						my_cont = FALSE;
+					
+					cnt = 0;
 				}
-
-				if (l)
-					g_static_rec_mutex_unlock (store->idle_lock);
-
-				if (resp)
-					g_free (resp);
-				resp = NULL;
-
-				/* If we are continuing the loop, handle idle_resp
-				 * now (this can invoke fetching new headers). */
-
-				if (store->idle_cont && idle_resp) {
-					process_idle_response (idle_resp);
-					idle_response_free (idle_resp);
-					idle_resp = NULL;
-					retval = NULL;
-				}
-			}
-
-			if (store->idle_cont)
-				first = TRUE;
-			else
-				my_cont = FALSE;
-
-			cnt = 0;
+				g_static_rec_mutex_unlock (store->idle_t_lock);
+			}			
+			CAMEL_SERVICE_REC_UNLOCK (store, connect_lock);
 		}
-
 		/* TNY TODO: try to use the select() of the non-blocking read
 		 * for this usleep() and cnt stuff. */
-
-		if (my_cont)
+		
+		if (!store->idle_kill && my_cont)
 			usleep (500000);
-
+		
 		cnt++;
 	}
 
@@ -4264,35 +4261,39 @@
 
 	if ((store->capabilities & IMAP_CAPABILITY_IDLE))
 	{
-		g_static_rec_mutex_lock (store->idle_t_lock);
 		if (store->in_idle && store->idle_thread && (g_thread_self () != store->idle_thread)) {
 			IdleResponse *idle_resp = NULL;
 			store->idle_cont = FALSE;
-			idle_resp = g_thread_join (store->idle_thread);
+			store->idle_kill = TRUE;
 
-			g_static_rec_mutex_lock (store->idle_lock);
-			g_static_rec_mutex_lock (store->idle_prefix_lock);
+			g_static_rec_mutex_lock (store->idle_t_lock);
+			if (store->idle_thread) {
+			/* camel_service_disconnect (CAMEL_SERVICE (store), FALSE, NULL); */
+				idle_resp = g_thread_join (store->idle_thread);
 
-			store->in_idle = FALSE;
-			store->idle_thread = NULL;
-			if (store->idle_prefix)
-				g_free (store->idle_prefix);
+				g_static_rec_mutex_lock (store->idle_lock);
+				g_static_rec_mutex_lock (store->idle_prefix_lock);
 
-			g_static_rec_mutex_unlock (store->idle_prefix_lock);
-			g_static_rec_mutex_unlock (store->idle_lock);
+				store->in_idle = FALSE;
+				store->idle_thread = NULL;
+				if (store->idle_prefix)
+					g_free (store->idle_prefix);
 
-			/* We are doing this here because here we wont hit the
-			 * priv->folder's lock of TnyCamelFolder during its
-			 * folder_changed handler. */
+				g_static_rec_mutex_unlock (store->idle_prefix_lock);
+				g_static_rec_mutex_unlock (store->idle_lock);
 
-			if (idle_resp) {
-				process_idle_response (idle_resp);
-				idle_response_free (idle_resp);
+				/* We are doing this here because here we wont hit the
+				 * priv->folder's lock of TnyCamelFolder during its
+				 * folder_changed handler. */
+
+				if (idle_resp) {
+					process_idle_response (idle_resp);
+					idle_response_free (idle_resp);
+				}
+				store->idle_prefix = NULL;
 			}
-
+			g_static_rec_mutex_unlock (store->idle_t_lock);
 		}
-		store->idle_prefix = NULL;
-		g_static_rec_mutex_unlock (store->idle_t_lock);
 
 	}
 
@@ -4326,7 +4327,7 @@
 		if (store->current_folder && !store->idle_prefix)
 		{
 			folder->folder_flags |= CAMEL_FOLDER_HAS_PUSHEMAIL_CAPABILITY;
-
+			store->idle_kill = TRUE;
 			g_static_rec_mutex_lock (store->idle_t_lock);
 
 			if (!store->in_idle && store->idle_thread && (g_thread_self () != store->idle_thread)) {