res_odbc.c: Allow concurrent access to request odbc connections

There are valid scenarios where res_odbc's connection pool might have some dead
or stuck connections while others are healthy (imagine network
elements/firewalls/routers silently timing out connections to a single DB and a
single IP address, or a heterogeneous connection pool connected to potentially
multiple IPs/instances of a replicated DB using a DNS front end for load
balancing and one replica fails).

In order to time out those unhealthy connections without blocking access to
other parts of Asterisk that may attempt access to the connection pool, it would
be beneficial to not lock/block access around the entire pool in
_ast_odbc_request_obj2 while doing potentially blocking operations on connection
pool objects such as the connection_dead() test, odbc_obj_connect(), or by
dereferencing a struct odbc_obj for the last time and triggering a
odbc_obj_disconnect().

This would facilitate much quicker and concurrent timeout of dead connections
via the connection_dead() test, which could block potentially for a long period
of time depending on odbc.ini or other odbc connector specific timeout settings.

This also would make rapid failover (in the clustered DB scenario) much quicker.

This patch changes the locking in _ast_odbc_request_obj2() to not lock around
odbc_obj_connect(), _disconnect(), and connection_dead(), while continuing to
lock around truly shared, non-immutable state like the connection_cnt member and
the connections list on struct odbc_class.

Fixes: #465

diff --git a/res/res_odbc.c b/res/res_odbc.c
index 54037f9ff0d4c78906e00fd69cadb443513b27ce..00e5b2d94bb87b4b0e269f80822c1557984060e9 100644 (file)
--- a/res/res_odbc.c
+++ b/res/res_odbc.c
@@ -915,12 +915,16 @@ struct odbc_obj *_ast_odbc_request_obj2(const char *name, struct ast_flags flags
                return NULL;
        }
 
-       ast_mutex_lock(&class->lock);
-
        while (!obj) {
+               ast_mutex_lock(&class->lock);
+
                obj = AST_LIST_REMOVE_HEAD(&class->connections, list);
 
+               ast_mutex_unlock(&class->lock);
+
                if (!obj) {
+                       ast_mutex_lock(&class->lock);
+
                        if (class->connection_cnt < class->maxconnections) {
                                /* If no connection is immediately available establish a new
                                 * one if allowed. If we try and fail we give up completely as
@@ -928,20 +932,31 @@ struct odbc_obj *_ast_odbc_request_obj2(const char *name, struct ast_flags flags
                                 */
                                obj = ao2_alloc(sizeof(*obj), odbc_obj_destructor);
                                if (!obj) {
+                                       ast_mutex_unlock(&class->lock);
                                        break;
                                }
 
                                obj->parent = ao2_bump(class);
+
+                               class->connection_cnt++;
+
+                               ast_mutex_unlock(&class->lock);
+
                                if (odbc_obj_connect(obj) == ODBC_FAIL) {
+                                       ast_mutex_lock(&class->lock);
+                                       class->connection_cnt--;
+                                       ast_mutex_unlock(&class->lock);
                                        ao2_ref(obj->parent, -1);
                                        ao2_ref(obj, -1);
                                        obj = NULL;
                                        break;
                                }
 
-                               class->connection_cnt++;
+                               ast_mutex_lock(&class->lock);
+
                                ast_debug(2, "Created ODBC handle %p on class '%s', new count is %zd\n", obj,
                                        name, class->connection_cnt);
+
                        } else {
                                /* Otherwise if we're not allowed to create a new one we
                                 * wait for another thread to give up the connection they
@@ -949,15 +964,24 @@ struct odbc_obj *_ast_odbc_request_obj2(const char *name, struct ast_flags flags
                                 */
                                ast_cond_wait(&class->cond, &class->lock);
                        }
+
+                       ast_mutex_unlock(&class->lock);
+
                } else if (connection_dead(obj, class)) {
                        /* If the connection is dead try to grab another functional one from the
                         * pool instead of trying to resurrect this one.
                         */
                        ao2_ref(obj, -1);
                        obj = NULL;
+
+                       ast_mutex_lock(&class->lock);
+
                        class->connection_cnt--;
                        ast_debug(2, "ODBC handle %p dead - removing from class '%s', new count is %zd\n",
                                obj, name, class->connection_cnt);
+
+                       ast_mutex_unlock(&class->lock);
+
                } else {
                        /* We successfully grabbed a connection from the pool and all is well!
                         */
@@ -966,7 +990,6 @@ struct odbc_obj *_ast_odbc_request_obj2(const char *name, struct ast_flags flags
                }
        }
 
-       ast_mutex_unlock(&class->lock);
        ao2_ref(class, -1);
 
        return obj;