Changeset 3801

Timestamp:

11/10/08 15:08:37

Author:

jdapena

Message:

Complete rework of the lock behavior of Tinymail (camel) imap IDLE
implementation.

The problem we want to fix is: sometimes IDLE thread causes
deathlocks (connect lock, idle lock, g_thread_join, etc).
This happens easily when there are disconnects, or
when we switch fast among folders.

The main changes are:
* Provided a count of reasons to stop idle because of connect lock.
* If a caller has taken connect lock, then it should do the send done

stuff itself.
* Idle thread has the connect lock always when it's in the body of the
loop.
* In any point, if we want to take the connect lock, then we should
stop idle. No partial idle can be alive when we want to connect.
* Camel queue enables/disables the possibility to start idle. That's
for avoiding running IDLE / SEND DONE in the middle of queued
operations.

Files:

• releases/modest/diablo-pe2/ChangeLog (modified) (1 diff)
• releases/modest/diablo-pe2/libtinymail-camel/camel-lite/camel/camel-service.c (modified) (3 diffs)
• releases/modest/diablo-pe2/libtinymail-camel/camel-lite/camel/camel-service.h (modified) (3 diffs)
• releases/modest/diablo-pe2/libtinymail-camel/camel-lite/camel/providers/imap/camel-imap-command.c (modified) (12 diffs)
• releases/modest/diablo-pe2/libtinymail-camel/camel-lite/camel/providers/imap/camel-imap-folder.c (modified) (44 diffs)
• releases/modest/diablo-pe2/libtinymail-camel/camel-lite/camel/providers/imap/camel-imap-folder.h (modified) (1 diff)
• releases/modest/diablo-pe2/libtinymail-camel/camel-lite/camel/providers/imap/camel-imap-search.c (modified) (2 diffs)
• releases/modest/diablo-pe2/libtinymail-camel/camel-lite/camel/providers/imap/camel-imap-store.c (modified) (60 diffs)
• releases/modest/diablo-pe2/libtinymail-camel/camel-lite/camel/providers/imap/camel-imap-store.h (modified) (4 diffs)
• releases/modest/diablo-pe2/libtinymail-camel/tny-camel-queue.c (modified) (4 diffs)

releases/modest/diablo-pe2/ChangeLog

@@ -1 +1 @@
-2008-11-10  Jose Dapena Paz  <jdapena@igalia.com>
+
+        Complete rework of the lock behavior of Tinymail
+        (camel) imap IDLE implementation.
+
+        The problem we want to fix is: sometimes IDLE thread causes
+        deathlocks (connect lock, idle lock, g_thread_join, etc).
+        This happens easily when there are disconnects, or
+        when we switch fast among folders.
+
+        The main changes are:
+        * Provided a count of reasons to stop idle because of connect lock.
+        * If a caller has taken connect lock, then it should do the send done
+        stuff itself.
+        * Idle thread has the connect lock always when it's in the body of the
+        loop.
+        * In any point, if we want to  take the connect lock, then we should
+        stop idle. No partial idle can be alive when we want to connect.
+        * Camel queue enables/disables the possibility to start idle. That's
+        for avoiding running IDLE / SEND DONE in the middle of queued
+        operations.
-
-        * Specify with defines the IDLE tick time (time between iterations in

releases/modest/diablo-pe2/libtinymail-camel/camel-lite/camel/camel-service.c

@@ -60 +60 @@
-static gboolean service_disconnect(CamelService *service, gboolean clean,
-                                   CamelException *ex);
+static void service_can_idle (CamelService *service, gboolean can_idle);
-static void cancel_connect (CamelService *service);
-static GList *query_auth_types (CamelService *service, CamelException *ex);
@@ -88 +89 @@
-        camel_service_class->get_name = get_name;
-        camel_service_class->get_path = get_path;
+        camel_service_class->can_idle = service_can_idle;
-}
-
@@ -770 +772 @@
-        return ret;
-}
+
+/**
+ * camel_service_can_idle:
+ * @service: a #CamelService
+ * @can_idle: a #gboolean
+ *
+ * Sets if service can do idle operations or not. This is for
+ * avoiding the service believe it can do idle operations in
+ * the middle of queued operations.
+ */
+void
+camel_service_can_idle (CamelService *service,
+                        gboolean can_idle)
+{
+        g_return_val_if_fail (CAMEL_IS_SERVICE (service), NULL);
+
+        CSERV_CLASS (service)->can_idle (service, can_idle);
+}
+
+static void
+service_can_idle (CamelService *service,
+                  gboolean can_idle)
+{
+        /* Default implementation is empty */
+}

releases/modest/diablo-pe2/libtinymail-camel/camel-lite/camel/camel-service.h

@@ -74 +74 @@
-        gboolean reconnecting;
-
+        gboolean can_idle;
+
-        con_op connecting;
-        con_op disconnecting;
@@ -102 +104 @@
-                                        gboolean brief);
-        char *    (*get_path)          (CamelService *service);
+        void      (*can_idle)          (CamelService *service, gboolean can_idle);
-
-} CamelServiceClass;
@@ -142 +145 @@
-                                                      CamelException *ex);
-
+void                camel_service_can_idle            (CamelService *service,
+                                                       gboolean can_idle);
+
-/* Standard Camel function */
-CamelType camel_service_get_type (void);

releases/modest/diablo-pe2/libtinymail-camel/camel-lite/camel/providers/imap/camel-imap-command.c

@@ -123 +123 @@
-        char *cmd = NULL;
-
-CAMEL_SERVICE_REC_LOCK (store, connect_lock
+camel_imap_store_stop_idle_connect_lock (store
-
-        if (fmt) {
@@ -180 +180 @@
-        if (!imap_command_start (store, folder, cmd, ex)) {
-                g_free (cmd);
-CAMEL_SERVICE_REC_UNLOCK (store, connect_lock
+camel_imap_store_connect_unlock_start_idle (store
-                return NULL;
-        }
@@ -235 +235 @@
-        va_end (ap);
-
-CAMEL_SERVICE_REC_LOCK (store, connect_lock
+camel_imap_store_stop_idle_connect_lock (store
-
-        ok = imap_command_start (store, folder, cmd, ex);
@@ -241 +241 @@
-
-        if (!ok)
-CAMEL_SERVICE_REC_UNLOCK (store, connect_lock
+camel_imap_store_connect_unlock_start_idle (store
-
-        return ok;
@@ -251 +251 @@
-{
-        ssize_t nwritten;
+        ssize_t nread;
-        gchar *resp = NULL;
-        CamelException myex = CAMEL_EXCEPTION_INITIALISER;
@@ -355 +356 @@
-
-        /* Read away whatever we got */
-camel_imap_store_readline_nb (store, &resp, &myex
+(nread = camel_imap_store_readline_nb (store, &resp, &myex)
-        {
+#ifdef IMAP_DEBUG
+                gchar *debug_resp;
+                gchar *debug_resp_escaped;
-                imap_debug ("unsolitcited: ");
-
+
+
+
+
+
-                imap_debug ("\n");
+#endif
-
-                g_free (resp);
@@ -435 +444 @@
-                        camel_imap_recon (store, &mex, TRUE);
-                        imap_debug ("Recon in cont: %s\n", camel_exception_get_description (&mex));
-CAMEL_SERVICE_REC_UNLOCK (store, connect_lock
+camel_imap_store_connect_unlock_start_idle (store
-                        camel_exception_clear (&mex);
-                        return NULL;
@@ -443 +452 @@
-                camel_service_disconnect (CAMEL_SERVICE (store), FALSE, NULL);
-
-CAMEL_SERVICE_REC_UNLOCK (store, connect_lock
+camel_imap_store_connect_unlock_start_idle (store
-                return NULL;
-        }
@@ -475 +484 @@
-
-        if (camel_imap_store_readline (store, &respbuf, ex) < 0) {
-CAMEL_SERVICE_REC_UNLOCK (store, connect_lock
+camel_imap_store_connect_unlock_start_idle (store
-                return CAMEL_IMAP_RESPONSE_ERROR;
-        }
@@ -538 +547 @@
-        if (type == CAMEL_IMAP_RESPONSE_ERROR ||
-            type == CAMEL_IMAP_RESPONSE_TAGGED)
-CAMEL_SERVICE_REC_UNLOCK (store, connect_lock
+camel_imap_store_connect_unlock_start_idle (store
-
-        return type;
@@ -623 +632 @@
-         */
-
-CAMEL_SERVICE_REC_LOCK (store, connect_lock
+camel_imap_store_stop_idle_connect_lock (store
-
-        response = g_new0 (CamelImapResponse, 1);
@@ -1070 +1079 @@
-        g_free (response);
-
-CAMEL_SERVICE_REC_UNLOCK (store, connect_lock
+camel_imap_store_connect_unlock_start_idle (store
-}
-

releases/modest/diablo-pe2/libtinymail-camel/camel-lite/camel/providers/imap/camel-imap-folder.c

@@ -913 +913 @@
-         * should do it.  */
-        CAMEL_FOLDER_REC_LOCK(folder, lock);
-CAMEL_SERVICE_REC_LOCK (imap_store, connect_lock
+camel_imap_store_stop_idle_connect_lock (imap_store
-
-        if (!camel_disco_store_check_online ((CamelDiscoStore*)imap_store, ex))
@@ -958 +958 @@
-        }
-done:
-        CAMEL_SERVICE_REC_UNLOCK (imap_store, connect_lock);
-        CAMEL_FOLDER_REC_UNLOCK(folder, lock);
-
-        camel_folder_summary_save(folder->summary, ex);
-        camel_store_summary_save((CamelStoreSummary *)((CamelImapStore *)folder->parent_store)->summary, ex);
-
-
+
+
-
-}
@@ -1498 +1496 @@
-
-        camel_exception_init (&local_ex);
-CAMEL_SERVICE_REC_LOCK (store, connect_lock
+camel_imap_store_stop_idle_connect_lock (store
-
-        /* Find a message with changed flags, find all of the other
@@ -1564 +1562 @@
-                g_ptr_array_free (matches, TRUE);
-
-                /* We unlock here so that other threads can have a chance to grab the connect_lock */
-                CAMEL_SERVICE_REC_UNLOCK (store, connect_lock);
-
-                /* check for an exception */
-                if (camel_exception_is_set (&local_ex)) {
-
+                        camel_imap_store_connect_unlock_start_idle (store);
+
-                        camel_exception_xfer (ex, &local_ex);
-                        camel_imap_folder_start_idle (folder);
-                        return;
-                }
-
-                /* Re-lock the connect_lock */
-                CAMEL_SERVICE_REC_LOCK (store, connect_lock);
-        }
-
@@ -1582 +1576 @@
-        imap_sync_offline (folder, ex);
-
-CAMEL_SERVICE_REC_UNLOCK (store, connect_lock
+camel_imap_store_connect_unlock_start_idle (store
-        camel_imap_folder_start_idle (folder);
-
@@ -1636 +1630 @@
-        char *set;
-
-CAMEL_SERVICE_REC_LOCK (store, connect_lock
+camel_imap_store_stop_idle_connect_lock (store
-
-        if ((store->capabilities & IMAP_CAPABILITY_UIDPLUS) == 0) {
-                ((CamelFolderClass *)CAMEL_OBJECT_GET_CLASS(folder))->sync(folder, 0, ex);
-                if (camel_exception_is_set(ex)) {
-CAMEL_SERVICE_REC_UNLOCK (store, connect_lock
+camel_imap_store_connect_unlock_start_idle (store
-                        return;
-                }
@@ -1656 +1650 @@
-                        camel_imap_response_free (store, response);
-                if (camel_exception_is_set (ex)) {
-CAMEL_SERVICE_REC_UNLOCK (store, connect_lock
+camel_imap_store_connect_unlock_start_idle (store
-                        g_free (set);
-                        return;
@@ -1683 +1677 @@
-        }
-
-
-
-
-
+
-}
-
@@ -1711 +1702 @@
-         * marked un-deleted. */
-
-CAMEL_SERVICE_REC_LOCK (store, connect_lock
+camel_imap_store_stop_idle_connect_lock (store
-
-        ((CamelFolderClass *)CAMEL_OBJECT_GET_CLASS(folder))->sync(folder, 0, ex);
-        if (camel_exception_is_set(ex)) {
-CAMEL_SERVICE_REC_UNLOCK (store, connect_lock
+camel_imap_store_connect_unlock_start_idle (store
-                return;
-        }
@@ -1726 +1717 @@
-        result = camel_imap_response_extract (store, response, "SEARCH", ex);
-        if (!result) {
-CAMEL_SERVICE_REC_UNLOCK (store, connect_lock
+camel_imap_store_connect_unlock_start_idle (store
-                return;
-        }
@@ -1786 +1777 @@
-                                g_ptr_array_free (keep_uids, TRUE);
-                                g_ptr_array_free (mark_uids, TRUE);
-CAMEL_SERVICE_REC_UNLOCK (store, connect_lock
+camel_imap_store_connect_unlock_start_idle (store
-                                return;
-                        }
@@ -1811 +1802 @@
-                                g_ptr_array_free (keep_uids, TRUE);
-                                g_ptr_array_free (mark_uids, TRUE);
+                                camel_imap_store_connect_unlock_start_idle (store);
-                                CAMEL_SERVICE_REC_UNLOCK (store, connect_lock);
-                                return;
@@ -1850 +1842 @@
-        g_free (result);
-
-
-
-
+
-
-}
@@ -2043 +2033 @@
-
-        /* Make sure a "folder_changed" is emitted. */
-CAMEL_SERVICE_REC_LOCK (store, connect_lock
+camel_imap_store_stop_idle_connect_lock (store
-        if (store->current_folder != folder ||
-            camel_folder_summary_count (folder->summary) == count)
-                imap_refresh_info (folder, ex);
-
-
-
+
-
-}
@@ -2105 +2093 @@
-         * to copy messages in the other direction from another thread.
-         */
-CAMEL_SERVICE_REC_LOCK (store, connect_lock
+camel_imap_store_stop_idle_connect_lock (store
-        CAMEL_IMAP_FOLDER_REC_LOCK (source, cache_lock);
-        CAMEL_IMAP_FOLDER_REC_LOCK (dest, cache_lock);
-CAMEL_SERVICE_REC_UNLOCK (store, connect_lock
+camel_imap_store_connect_unlock_start_idle (store
-
-        if (transferred_uids) {
@@ -2812 +2800 @@
-                                int i;
-
-CAMEL_SERVICE_REC_LOCK(store, connect_lock
+camel_imap_store_stop_idle_connect_lock (store
-                                if (!camel_disco_store_check_online ((CamelDiscoStore*)store, ex)) {
-CAMEL_SERVICE_REC_UNLOCK(store, connect_lock
+camel_imap_store_connect_unlock_start_idle (store
-                                        camel_exception_set (ex, CAMEL_EXCEPTION_FOLDER_UID_NOT_AVAILABLE,
-                                                             _("This message is not currently available"));
@@ -2821 +2809 @@
-
-                                response = camel_imap_command (store, folder, ex, "UID FETCH %s BODY", uid);
-CAMEL_SERVICE_REC_UNLOCK(store, connect_lock
+camel_imap_store_connect_unlock_start_idle (store
-
-                                if (response) {
@@ -3369 +3357 @@
-                if (tcnt < (exists - seq))
-                {
-                        int i;
-
-                        g_ptr_array_foreach (needheaders, (GFunc)g_free, NULL);
@@ -3878 +3865 @@
-}
-
+static void
+process_idle_body (CamelImapStore *store, CamelFolder *folder, IdleResponse **idle_response, CamelException *exception)
+{
+        char *resp = NULL;
+
+        while (camel_imap_store_readline_nb (store, &resp, exception) > 0) {
+                if (resp && strlen (resp) > 1 && resp[0] == '*') {
+                        if (!*idle_response)
+                                *idle_response = idle_response_new (folder);
+                        consume_idle_line (store, folder, resp, *idle_response);
+                }
+                
+                if (resp)
+                        g_free (resp);
+                resp = NULL;
+        }
+}
+
+static void
+process_idle_untagged_response (CamelImapStore *store, CamelFolder *folder, IdleResponse **idle_response, CamelException *exception)
+{
+        char *resp = NULL;
+        CamelImapResponseType type;
+
+        while ((type = camel_imap_command_response_idle (store, &resp, exception)) == CAMEL_IMAP_RESPONSE_UNTAGGED) {
+                if (resp && strlen (resp) > 1 && resp[0] == '*') {
+                        if (!*idle_response)
+                                *idle_response = idle_response_new (folder);
+                        consume_idle_line (store, folder, resp, *idle_response);
+                }
+                
+                if (resp)
+                        g_free (resp);
+                resp = NULL;
+        }
+}
+
+static void
+do_send_done (CamelImapStore *store, CamelFolder *folder, IdleResponse **idle_resp, CamelException *ex)
+{
+        if (store->idle_prefix) {
+                gboolean l = g_static_rec_mutex_trylock (store->idle_lock);
+
+                if (!store->idle_kill) {
+                        int nwritten=0;
+                        /* 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). */
+
+                        process_idle_body (store, folder, idle_resp, ex);
+
+                        /* 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 */
+
+                        process_idle_untagged_response (store, folder, idle_resp, ex);
+                }
+
+                if (l)
+                        g_static_rec_mutex_unlock (store->idle_lock);
+
+                /* 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;
+                }
+
+        }
+        store->idle_send_done_happened = TRUE;
+}
+
+static gpointer
+send_done_in_stop_idle (CamelImapStore *store, CamelFolder *folder)
+{
+        CamelException ex = CAMEL_EXCEPTION_INITIALISER;
+        IdleResponse *idle_resp = NULL;
+
+        /* This method is used for sending the commands we expect idle thread to do to finish
+         * idle loop, when the caller thread has the connect lock (and then the idle thread cannot
+         * do safely these commands */
+
+        if (g_static_rec_mutex_trylock (store->idle_lock)) {
+                        /* Step B) (see idle_thread) */
+
+                        if (!store->idle_kill) {
+                                process_idle_body (store, folder, &idle_resp, &ex);
+                        }
+                        g_static_rec_mutex_unlock (store->idle_lock);
+        }
+
+        /* Step C) (see idle_thread). We're assuming idle_cont == FALSE, send_done == TRUE*/
+        do_send_done (store, folder, &idle_resp, &ex);
+        return idle_resp;
+}
+
-typedef struct {
-        CamelFolder *folder;
@@ -3894 +3984 @@
-        gboolean tfirst = TRUE, first = TRUE, my_cont, had_cond = FALSE;
-        int cnt = 0;
-        int nwritten=0;
-        gpointer retval = NULL;
-
@@ -3957 +4046 @@
-        }
-
+        store->idle_send_done_happened = FALSE;
-        /* While nothing has stopped us yet ...
-         * TNY TODO: it would be nicer to use select() here, rather than usleep() */
-
-
+
+
+
-        {
-                CamelException ex = CAMEL_EXCEPTION_INITIALISER;
-                char *resp = NULL;
-                IdleResponse *idle_resp = NULL;
-                gboolean senddone = FALSE;
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
+
+
+
+
+
+
+
+
-                                gboolean l = g_static_rec_mutex_trylock (store->idle_lock);
-
-                                if (!store->idle_kill)
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
+
-                                if (l)
-                                        g_static_rec_mutex_unlock (store->idle_lock);
-
-
-
-
-
-
-
-
-
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
-                                        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;
-                                }
-
-
-
-
-
-
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+