[geary/wip/745561-hang] Potential fix to hang problem

[Jim Nelson <jnelson src gnome org>]

commit 82fd668043c19f642a45ae1d3068a7ec55360208
Author: Jim Nelson <jim yorba org>
Date:   Mon Mar 30 17:18:26 2015 -0700

    Potential fix to hang problem
    
     * Geary.Endpoint uses a 30s connect timeout and an app-specific
       activity timeout (10m in case of IMAP).
     * ClientSessionManager is refactored to avoid locking issues with
       sessions mutex.

 src/engine/api/geary-endpoint.vala                 |   20 +++-
 .../transport/imap-client-session-manager.vala     |  109 ++++++++------------
 2 files changed, 59 insertions(+), 70 deletions(-)
---
diff --git a/src/engine/api/geary-endpoint.vala b/src/engine/api/geary-endpoint.vala
index f9be15a..bf07ffc 100644
--- a/src/engine/api/geary-endpoint.vala
+++ b/src/engine/api/geary-endpoint.vala
@@ -12,6 +12,11 @@
 public class Geary.Endpoint : BaseObject {
     public const string PROP_TRUST_UNTRUSTED_HOST = "trust-untrusted-host";
     
+    /**
+     * Recommended { link connect_timeout_sec}.
+     */
+    public const uint DEFAULT_CONNECT_TIMEOUT_SEC = 30;
+    
     [Flags]
     public enum Flags {
         NONE = 0,
@@ -42,7 +47,8 @@ public class Geary.Endpoint : BaseObject {
     
     public NetworkAddress remote_address { get; private set; }
     public Flags flags { get; private set; }
-    public uint timeout_sec { get; private set; }
+    public uint connect_timeout_sec { get; private set; default = DEFAULT_CONNECT_TIMEOUT_SEC; }
+    public uint activity_timeout_sec { get; private set; }
     public TlsCertificateFlags tls_validation_flags { get; set; default = TlsCertificateFlags.VALIDATE_ALL; }
     public bool force_ssl3 { get; set; default = false; }
     
@@ -114,10 +120,10 @@ public class Geary.Endpoint : BaseObject {
      */
     public signal void untrusted_host(SecurityType security, TlsConnection cx);
     
-    public Endpoint(string host_specifier, uint16 default_port, Flags flags, uint timeout_sec) {
+    public Endpoint(string host_specifier, uint16 default_port, Flags flags, uint activity_timeout_sec) {
         this.remote_address = new NetworkAddress(host_specifier, default_port);
         this.flags = flags;
-        this.timeout_sec = timeout_sec;
+        this.activity_timeout_sec = activity_timeout_sec;
     }
     
     private SocketClient get_socket_client() {
@@ -132,7 +138,9 @@ public class Geary.Endpoint : BaseObject {
             socket_client.event.connect(on_socket_client_event);
         }
         
-        socket_client.set_timeout(timeout_sec);
+        // Use the connect_timeout_sec for SocketClient, then set the activity timeout when the
+        // Socket is available
+        socket_client.set_timeout(connect_timeout_sec);
         
         return socket_client;
     }
@@ -144,6 +152,10 @@ public class Geary.Endpoint : BaseObject {
         if (tcp != null)
             tcp.set_graceful_disconnect(flags.is_all_set(Flags.GRACEFUL_DISCONNECT));
         
+        Socket? socket = cx.get_socket();
+        if (socket != null)
+            socket.set_timeout(activity_timeout_sec);
+        
         return cx;
     }
     
diff --git a/src/engine/imap/transport/imap-client-session-manager.vala 
b/src/engine/imap/transport/imap-client-session-manager.vala
index c4f577d..b4f596b 100644
--- a/src/engine/imap/transport/imap-client-session-manager.vala
+++ b/src/engine/imap/transport/imap-client-session-manager.vala
@@ -58,7 +58,6 @@ public class Geary.Imap.ClientSessionManager : BaseObject {
     private Endpoint endpoint;
     private NetworkMonitor network_monitor;
     private Gee.HashSet<ClientSession> sessions = new Gee.HashSet<ClientSession>();
-    private int pending_sessions = 0;
     private Nonblocking.Mutex sessions_mutex = new Nonblocking.Mutex();
     private Gee.HashSet<ClientSession> reserved_sessions = new Gee.HashSet<ClientSession>();
     private bool authentication_failed = false;
@@ -159,48 +158,48 @@ public class Geary.Imap.ClientSessionManager : BaseObject {
         if (!is_open)
             return;
         
-        int token;
-        try {
-            token = yield sessions_mutex.claim_async();
-        } catch (Error claim_err) {
-            debug("Unable to claim session table mutex for adjusting pool: %s", claim_err.message);
-            
-            return;
-        }
-        
-        while ((sessions.size + pending_sessions) < min_pool_size
+        bool try_again = true;
+        while (sessions.size < min_pool_size
             && !authentication_failed
             && is_open
             && !untrusted_host
-            && is_endpoint_reachable) {
-            pending_sessions++;
-            create_new_authorized_session.begin(null, on_created_new_authorized_session);
-        }
-        
-        try {
-            sessions_mutex.release(ref token);
-        } catch (Error release_err) {
-            debug("Unable to release session table mutex after adjusting pool: %s", release_err.message);
-        }
-    }
-    
-    private void on_created_new_authorized_session(Object? source, AsyncResult result) {
-        pending_sessions--;
-        
-        try {
-            create_new_authorized_session.end(result);
-        } catch (Error err) {
-            debug("Unable to create authorized session to %s: %s", endpoint.to_string(), err.message);
-            
-            // try again after a slight delay and bump up delay
-            if (authorized_session_error_retry_timeout_id != 0)
-                Source.remove(authorized_session_error_retry_timeout_id);
+            && is_endpoint_reachable
+            && try_again) {
+            // acquire sessions table lock for each session creation ... this means we can't launch
+            // several connects at startup, but it's also problematic to keep the session table
+            // locked in that situation, so need to serialize this
+            int token;
+            try {
+                token = yield sessions_mutex.claim_async();
+            } catch (Error claim_err) {
+                debug("Unable to claim session table mutex for adjusting pool: %s", claim_err.message);
+                
+                break;
+            }
             
-            authorized_session_error_retry_timeout_id = Timeout.add_seconds(
-                authorized_session_retry_sec, on_authorized_session_error_retry_timeout);
+            try {
+                yield create_new_authorized_session(null);
+            } catch (Error err) {
+                debug("Unable to create authorized session to %s: %s", endpoint.to_string(), err.message);
+                
+                // try again after a slight delay and bump up delay
+                if (authorized_session_error_retry_timeout_id != 0)
+                    Source.remove(authorized_session_error_retry_timeout_id);
+                
+                authorized_session_error_retry_timeout_id = Timeout.add_seconds(
+                    authorized_session_retry_sec, on_authorized_session_error_retry_timeout);
+                
+                authorized_session_retry_sec = (authorized_session_retry_sec * 2).clamp(
+                    AUTHORIZED_SESSION_ERROR_MIN_RETRY_TIMEOUT_SEC, 
AUTHORIZED_SESSION_ERROR_MAX_RETRY_TIMEOUT_SEC);
+                
+                try_again = false;
+            }
             
-            authorized_session_retry_sec = (authorized_session_retry_sec * 2).clamp(
-                AUTHORIZED_SESSION_ERROR_MIN_RETRY_TIMEOUT_SEC, 
AUTHORIZED_SESSION_ERROR_MAX_RETRY_TIMEOUT_SEC);
+            try {
+                sessions_mutex.release(ref token);
+            } catch (Error release_err) {
+                debug("Unable to release session table mutex after adjusting pool: %s", release_err.message);
+            }
         }
     }
     
@@ -212,7 +211,10 @@ public class Geary.Imap.ClientSessionManager : BaseObject {
         return false;
     }
     
+    // This *must* be called with the sessions_mutex locked
     private async ClientSession create_new_authorized_session(Cancellable? cancellable) throws Error {
+        assert(sessions_mutex.is_locked());
+        
         if (authentication_failed)
             throw new ImapError.UNAUTHENTICATED("Invalid ClientSessionManager credentials");
         
@@ -226,21 +228,14 @@ public class Geary.Imap.ClientSessionManager : BaseObject {
         
         // add session to pool before launching all the connect activity so error cases can properly
         // back it out
-        if (sessions_mutex.is_locked())
-            locked_add_session(new_session);
-        else
-            yield unlocked_add_session_async(new_session);
+        locked_add_session(new_session);
         
         try {
             yield new_session.connect_async(cancellable);
         } catch (Error err) {
             debug("[%s] Connect failure: %s", new_session.to_string(), err.message);
             
-            bool removed;
-            if (sessions_mutex.is_locked())
-                removed = locked_remove_session(new_session);
-            else
-                removed = yield unlocked_remove_session_async(new_session);
+            bool removed = locked_remove_session(new_session);
             assert(removed);
             
             throw err;
@@ -260,11 +255,7 @@ public class Geary.Imap.ClientSessionManager : BaseObject {
                     new_session.to_string(), disconnect_err.message);
             }
             
-            bool removed;
-            if (sessions_mutex.is_locked())
-                removed = locked_remove_session(new_session);
-            else
-                removed = yield unlocked_remove_session_async(new_session);
+            bool removed = locked_remove_session(new_session);
             assert(removed);
             
             throw err;
@@ -459,12 +450,6 @@ public class Geary.Imap.ClientSessionManager : BaseObject {
         session.login_failed.connect(on_login_failed);
     }
     
-    private async void unlocked_add_session_async(ClientSession session) throws Error {
-        int token = yield sessions_mutex.claim_async();
-        locked_add_session(session);
-        sessions_mutex.release(ref token);
-    }
-    
     // Only call with sessions mutex locked
     private bool locked_remove_session(ClientSession session) {
         bool removed = sessions.remove(session);
@@ -478,14 +463,6 @@ public class Geary.Imap.ClientSessionManager : BaseObject {
         return removed;
     }
     
-    private async bool unlocked_remove_session_async(ClientSession session) throws Error {
-        int token = yield sessions_mutex.claim_async();
-        bool removed = locked_remove_session(session);
-        sessions_mutex.release(ref token);
-        
-        return removed;
-    }
-    
     private void on_imap_untrusted_host() {
         // this is called any time trust issues are detected, so immediately clutch in to stop
         // retries