Index: include/asterisk/res_odbc.h =================================================================== --- include/asterisk/res_odbc.h (revision 348047) +++ include/asterisk/res_odbc.h (working copy) @@ -44,7 +44,7 @@ enum { /*! \brief ODBC container */ struct odbc_obj { - ast_mutex_t lock; + ast_rwlock_t lock; SQLHDBC con; /*!< ODBC Connection Handle */ struct odbc_class *parent; /*!< Information about the connection is protected */ struct timeval last_used; /*!< Used by idlecheck to determine if the connection should be renegotiated */ Index: res/res_odbc.c =================================================================== --- res/res_odbc.c (revision 348047) +++ res/res_odbc.c (working copy) @@ -416,7 +416,7 @@ static void odbc_obj_destructor(void *data) struct odbc_class *class = obj->parent; obj->parent = NULL; odbc_obj_disconnect(obj); - ast_mutex_destroy(&obj->lock); + ast_rwlock_destroy(&obj->lock); ao2_ref(class, -1); } @@ -473,6 +473,7 @@ struct odbc_cache_tables *ast_odbc_find_table(cons } /* Table structure not already cached; build it now. */ + ast_rwlock_rdlock(&obj->lock); do { res = SQLAllocHandle(SQL_HANDLE_STMT, obj->con, &stmt); if ((res != SQL_SUCCESS) && (res != SQL_SUCCESS_WITH_INFO)) { @@ -543,6 +544,7 @@ struct odbc_cache_tables *ast_odbc_find_table(cons AST_RWLIST_RDLOCK(&(tableptr->columns)); break; } while (1); + ast_rwlock_unlock(&obj->lock); AST_RWLIST_UNLOCK(&odbc_tables); @@ -589,6 +591,8 @@ SQLHSTMT ast_odbc_direct_execute(struct odbc_obj * int attempt; SQLHSTMT stmt; + ast_rwlock_rdlock(&obj->lock); + for (attempt = 0; attempt < 2; attempt++) { stmt = exec_cb(obj, data); @@ -605,6 +609,8 @@ SQLHSTMT ast_odbc_direct_execute(struct odbc_obj * } } + ast_rwlock_unlock(&obj->lock); + return stmt; } @@ -616,7 +622,10 @@ SQLHSTMT ast_odbc_prepare_and_execute(struct odbc_ unsigned char state[10], diagnostic[256]; SQLHSTMT stmt; + ast_rwlock_rdlock(&obj->lock); + for (attempt = 0; attempt < 2; attempt++) { + /* This prepare callback may do more than just prepare -- it may also * bind parameters, bind results, etc. The real key, here, is that * when we disconnect, all handles become invalid for most databases. @@ -666,6 +675,8 @@ SQLHSTMT ast_odbc_prepare_and_execute(struct odbc_ } } + ast_rwlock_unlock(&obj->lock); + return stmt; } @@ -676,6 +687,8 @@ int ast_odbc_smart_execute(struct odbc_obj *obj, S SQLSMALLINT diagbytes=0; unsigned char state[10], diagnostic[256]; + ast_rwlock_rdlock(&obj->lock); + res = SQLExecute(stmt); if ((res != SQL_SUCCESS) && (res != SQL_SUCCESS_WITH_INFO) && (res != SQL_NO_DATA)) { if (res == SQL_ERROR) { @@ -693,6 +706,8 @@ int ast_odbc_smart_execute(struct odbc_obj *obj, S obj->last_used = ast_tvnow(); } + ast_rwlock_unlock(&obj->lock); + return res; } @@ -713,6 +728,7 @@ SQLRETURN ast_odbc_ast_str_SQLGetData(struct ast_s return res; } +/* We expect a read lock on obj->lock here */ int ast_odbc_sanity_check(struct odbc_obj *obj) { char *test_sql = "select 1"; @@ -740,10 +756,19 @@ int ast_odbc_sanity_check(struct odbc_obj *obj) SQLFreeHandle (SQL_HANDLE_STMT, stmt); } - if (!obj->up && !obj->tx) { /* Try to reconnect! */ + /* Try to reconnect if not up */ + if (!obj->up && !obj->tx) { + /* Bah, need to upgrade to a write lock */ + ast_rwlock_unlock(&obj->lock); ast_log(LOG_WARNING, "Connection is down attempting to reconnect...\n"); - odbc_obj_disconnect(obj); - odbc_obj_connect(obj); + ast_rwlock_wrlock(&obj->lock); + /* Double check that someone else didn't fix it for us already. */ + if (!obj->up && !obj->tx) { + /* Call disconnect explicitly, since obj->up makes connect + * think that that is done already. */ + odbc_obj_disconnect(obj); + odbc_obj_connect(obj); + } } return obj->up; } @@ -966,7 +991,7 @@ static char *handle_cli_odbc_show(struct ast_cli_e ast_cli(a->fd, " Pooled: Yes\n Limit: %d\n Connections in use: %d\n", class->limit, class->count); while ((current = ao2_iterator_next(&aoi2))) { - ast_mutex_lock(¤t->lock); + ast_rwlock_rdlock(¤t->lock); #ifdef DEBUG_THREADS ast_cli(a->fd, " - Connection %d: %s (%s:%d %s)\n", ++count, current->used ? "in use" : @@ -977,7 +1002,7 @@ static char *handle_cli_odbc_show(struct ast_cli_e current->used ? "in use" : current->up && ast_odbc_sanity_check(current) ? "connected" : "disconnected"); #endif - ast_mutex_unlock(¤t->lock); + ast_rwlock_unlock(¤t->lock); ao2_ref(current, -1); } ao2_iterator_destroy(&aoi2); @@ -1032,9 +1057,11 @@ static void odbc_release_obj2(struct odbc_obj *obj SQLSMALLINT diagbytes=0, i; unsigned char state[10], diagnostic[256]; + ast_debug(2, "odbc_release_obj2(%p) called (obj->txf = %p)\n", obj, obj->txf); if (tx) { ast_debug(1, "called on a transactional handle with %s\n", tx->forcecommit ? "COMMIT" : "ROLLBACK"); + ast_rwlock_rdlock(&obj->lock); if (SQLEndTran(SQL_HANDLE_DBC, obj->con, tx->forcecommit ? SQL_COMMIT : SQL_ROLLBACK) == SQL_ERROR) { /* Handle possible transaction commit failure */ SQLGetDiagField(SQL_HANDLE_DBC, obj->con, 1, SQL_DIAG_NUMBER, &numfields, SQL_IS_INTEGER, &diagbytes); @@ -1066,6 +1093,7 @@ static void odbc_release_obj2(struct odbc_obj *obj } } } + ast_rwlock_unlock(&obj->lock); } #ifdef DEBUG_THREADS @@ -1112,6 +1140,7 @@ static int commit_exec(struct ast_channel *chan, c pbx_builtin_setvar_helper(chan, "COMMIT_RESULT", "OK"); if (tx) { + ast_rwlock_rdlock(&tx->obj->lock); if (SQLEndTran(SQL_HANDLE_DBC, tx->obj->con, SQL_COMMIT) == SQL_ERROR) { struct ast_str *errors = ast_str_thread_get(&errors_buf, 16); ast_str_reset(errors); @@ -1129,6 +1158,7 @@ static int commit_exec(struct ast_channel *chan, c } pbx_builtin_setvar_helper(chan, "COMMIT_RESULT", ast_str_buffer(errors)); } + ast_rwlock_unlock(&tx->obj->lock); } return 0; } @@ -1149,6 +1179,7 @@ static int rollback_exec(struct ast_channel *chan, pbx_builtin_setvar_helper(chan, "ROLLBACK_RESULT", "OK"); if (tx) { + ast_rwlock_rdlock(&tx->obj->lock); if (SQLEndTran(SQL_HANDLE_DBC, tx->obj->con, SQL_ROLLBACK) == SQL_ERROR) { struct ast_str *errors = ast_str_thread_get(&errors_buf, 16); ast_str_reset(errors); @@ -1166,6 +1197,7 @@ static int rollback_exec(struct ast_channel *chan, } pbx_builtin_setvar_helper(chan, "ROLLBACK_RESULT", ast_str_buffer(errors)); } + ast_rwlock_unlock(&tx->obj->lock); } return 0; } @@ -1187,16 +1219,25 @@ static int aoro2_class_cb(void *obj, void *arg, in static int aoro2_obj_cb(void *vobj, void *arg, int flags) { struct odbc_obj *obj = vobj; - ast_mutex_lock(&obj->lock); + ast_rwlock_wrlock(&obj->lock); if ((arg == NO_TX && !obj->tx) || (arg == EOR_TX && !obj->used) || (arg == USE_TX && obj->tx && !obj->used)) { obj->used = 1; - ast_mutex_unlock(&obj->lock); + ast_rwlock_unlock(&obj->lock); return CMP_MATCH | CMP_STOP; } - ast_mutex_unlock(&obj->lock); + ast_rwlock_unlock(&obj->lock); return 0; } +static int aoro2_obj_notx_cb(void *vobj, void *arg, int flags) +{ + struct odbc_obj *obj = vobj; + if (!obj->tx) { + return CMP_MATCH | CMP_STOP; + } + return 0; +} + struct odbc_obj *_ast_odbc_request_obj2(const char *name, struct ast_flags flags, const char *file, const char *function, int lineno) { struct odbc_obj *obj = NULL; @@ -1230,7 +1271,7 @@ struct odbc_obj *_ast_odbc_request_obj2(const char return NULL; } ast_assert(ao2_ref(obj, 0) == 1); - ast_mutex_init(&obj->lock); + ast_rwlock_init(&obj->lock); /* obj inherits the outstanding reference to class */ obj->parent = class; class = NULL; @@ -1255,7 +1296,13 @@ struct odbc_obj *_ast_odbc_request_obj2(const char class = NULL; } - if (obj && ast_test_flag(&flags, RES_ODBC_INDEPENDENT_CONNECTION)) { + if (!obj) { + return NULL; + } + + ast_rwlock_wrlock(&obj->lock); + + if (ast_test_flag(&flags, RES_ODBC_INDEPENDENT_CONNECTION)) { /* Ensure this connection has autocommit turned off. */ if (SQLSetConnectAttr(obj->con, SQL_ATTR_AUTOCOMMIT, (void *)SQL_AUTOCOMMIT_OFF, 0) == SQL_ERROR) { SQLGetDiagField(SQL_HANDLE_DBC, obj->con, 1, SQL_DIAG_NUMBER, &numfields, SQL_IS_INTEGER, &diagbytes); @@ -1279,7 +1326,7 @@ struct odbc_obj *_ast_odbc_request_obj2(const char ast_debug(3, "Unable to allocate object\n"); return NULL; } - ast_mutex_init(&obj->lock); + ast_rwlock_init(&obj->lock); /* obj inherits the outstanding reference to class */ obj->parent = class; class = NULL; @@ -1294,7 +1341,13 @@ struct odbc_obj *_ast_odbc_request_obj2(const char } } - if (obj && SQLSetConnectAttr(obj->con, SQL_ATTR_AUTOCOMMIT, (void *)SQL_AUTOCOMMIT_OFF, 0) == SQL_ERROR) { + if (!obj) { + return NULL; + } + + ast_rwlock_wrlock(&obj->lock); + + if (SQLSetConnectAttr(obj->con, SQL_ATTR_AUTOCOMMIT, (void *)SQL_AUTOCOMMIT_OFF, 0) == SQL_ERROR) { SQLGetDiagField(SQL_HANDLE_DBC, obj->con, 1, SQL_DIAG_NUMBER, &numfields, SQL_IS_INTEGER, &diagbytes); for (i = 0; i < numfields; i++) { SQLGetDiagRec(SQL_HANDLE_DBC, obj->con, i + 1, state, &nativeerror, diagnostic, sizeof(diagnostic), &diagbytes); @@ -1307,7 +1360,7 @@ struct odbc_obj *_ast_odbc_request_obj2(const char } } else { /* Non-pooled connection: multiple modules can use the same connection. */ - if ((obj = ao2_callback(class->obj_container, 0, aoro2_obj_cb, NO_TX))) { + if ((obj = ao2_callback(class->obj_container, 0, aoro2_obj_notx_cb, NO_TX))) { /* Object is not constructed, so delete outstanding reference to class. */ ast_assert(ao2_ref(class, 0) > 1); ao2_ref(class, -1); @@ -1320,7 +1373,7 @@ struct odbc_obj *_ast_odbc_request_obj2(const char ast_debug(3, "Unable to allocate object\n"); return NULL; } - ast_mutex_init(&obj->lock); + ast_rwlock_init(&obj->lock); /* obj inherits the outstanding reference to class */ obj->parent = class; class = NULL; @@ -1334,7 +1387,13 @@ struct odbc_obj *_ast_odbc_request_obj2(const char } } - if (obj && SQLSetConnectAttr(obj->con, SQL_ATTR_AUTOCOMMIT, (void *)SQL_AUTOCOMMIT_ON, 0) == SQL_ERROR) { + if (!obj) { + return NULL; + } + + ast_rwlock_wrlock(&obj->lock); + + if (SQLSetConnectAttr(obj->con, SQL_ATTR_AUTOCOMMIT, (void *)SQL_AUTOCOMMIT_ON, 0) == SQL_ERROR) { SQLGetDiagField(SQL_HANDLE_DBC, obj->con, 1, SQL_DIAG_NUMBER, &numfields, SQL_IS_INTEGER, &diagbytes); for (i = 0; i < numfields; i++) { SQLGetDiagRec(SQL_HANDLE_DBC, obj->con, i + 1, state, &nativeerror, diagnostic, sizeof(diagnostic), &diagbytes); @@ -1347,8 +1406,10 @@ struct odbc_obj *_ast_odbc_request_obj2(const char } } + ast_assert(obj != NULL); /* .. and it is locked too */ + /* Set the isolation property */ - if (obj && SQLSetConnectAttr(obj->con, SQL_ATTR_TXN_ISOLATION, (void *)(long)obj->parent->isolation, 0) == SQL_ERROR) { + if (SQLSetConnectAttr(obj->con, SQL_ATTR_TXN_ISOLATION, (void *)(long)obj->parent->isolation, 0) == SQL_ERROR) { SQLGetDiagField(SQL_HANDLE_DBC, obj->con, 1, SQL_DIAG_NUMBER, &numfields, SQL_IS_INTEGER, &diagbytes); for (i = 0; i < numfields; i++) { SQLGetDiagRec(SQL_HANDLE_DBC, obj->con, i + 1, state, &nativeerror, diagnostic, sizeof(diagnostic), &diagbytes); @@ -1360,29 +1421,28 @@ struct odbc_obj *_ast_odbc_request_obj2(const char } } - if (obj && ast_test_flag(&flags, RES_ODBC_CONNECTED) && !obj->up) { + if (ast_test_flag(&flags, RES_ODBC_CONNECTED) && !obj->up) { /* Check if this connection qualifies for reconnection, with negative connection cache time */ if (time(NULL) > obj->parent->last_negative_connect.tv_sec + obj->parent->negative_connection_cache.tv_sec) { odbc_obj_connect(obj); } - } else if (obj && ast_test_flag(&flags, RES_ODBC_SANITY_CHECK)) { + } else if (ast_test_flag(&flags, RES_ODBC_SANITY_CHECK)) { ast_odbc_sanity_check(obj); - } else if (obj && obj->parent->idlecheck > 0 && ast_tvdiff_sec(ast_tvnow(), obj->last_used) > obj->parent->idlecheck) { + } else if (obj->parent->idlecheck > 0 && ast_tvdiff_sec(ast_tvnow(), obj->last_used) > obj->parent->idlecheck) { odbc_obj_connect(obj); } + /* We had it WR-locked because of the obj_connects we see here. */ + ast_rwlock_unlock(&obj->lock); + #ifdef DEBUG_THREADS - if (obj) { - ast_copy_string(obj->file, file, sizeof(obj->file)); - ast_copy_string(obj->function, function, sizeof(obj->function)); - obj->lineno = lineno; - } + ast_copy_string(obj->file, file, sizeof(obj->file)); + ast_copy_string(obj->function, function, sizeof(obj->function)); + obj->lineno = lineno; #endif ast_assert(class == NULL); - if (obj) { - ast_assert(ao2_ref(obj, 0) > 1); - } + ast_assert(ao2_ref(obj, 0) > 1); return obj; } @@ -1430,16 +1490,17 @@ static odbc_status odbc_obj_disconnect(struct odbc SQLINTEGER err; short int mlen; unsigned char msg[200], state[10]; + SQLHDBC con; /* Nothing to disconnect */ if (!obj->con) { return ODBC_SUCCESS; } - ast_mutex_lock(&obj->lock); + con = obj->con; + obj->con = NULL; + res = SQLDisconnect(con); - res = SQLDisconnect(obj->con); - if (obj->parent) { if (res == SQL_SUCCESS || res == SQL_SUCCESS_WITH_INFO) { ast_log(LOG_DEBUG, "Disconnected %d from %s [%s]\n", res, obj->parent->name, obj->parent->dsn); @@ -1448,16 +1509,14 @@ static odbc_status odbc_obj_disconnect(struct odbc } } - if ((res = SQLFreeHandle(SQL_HANDLE_DBC, obj->con) == SQL_SUCCESS)) { - obj->con = NULL; - ast_log(LOG_DEBUG, "Database handle deallocated\n"); + if ((res = SQLFreeHandle(SQL_HANDLE_DBC, con) == SQL_SUCCESS)) { + ast_log(LOG_DEBUG, "Database handle %p deallocated\n", con); } else { - SQLGetDiagRec(SQL_HANDLE_DBC, obj->con, 1, state, &err, msg, 100, &mlen); - ast_log(LOG_WARNING, "Unable to deallocate database handle? %d errno=%d %s\n", res, (int)err, msg); + SQLGetDiagRec(SQL_HANDLE_DBC, con, 1, state, &err, msg, 100, &mlen); + ast_log(LOG_WARNING, "Unable to deallocate database handle %p? %d errno=%d %s\n", obj->con, res, (int)err, msg); } obj->up = 0; - ast_mutex_unlock(&obj->lock); return ODBC_SUCCESS; } @@ -1471,40 +1530,43 @@ static odbc_status odbc_obj_connect(struct odbc_ob SQLINTEGER enable = 1; char *tracefile = "/tmp/odbc.trace"; #endif - ast_mutex_lock(&obj->lock); + SQLHDBC con; if (obj->up) { odbc_obj_disconnect(obj); ast_log(LOG_NOTICE, "Re-connecting %s\n", obj->parent->name); } else { + ast_assert(obj->con == NULL); ast_log(LOG_NOTICE, "Connecting %s\n", obj->parent->name); } - res = SQLAllocHandle(SQL_HANDLE_DBC, obj->parent->env, &obj->con); + res = SQLAllocHandle(SQL_HANDLE_DBC, obj->parent->env, &con); if ((res != SQL_SUCCESS) && (res != SQL_SUCCESS_WITH_INFO)) { ast_log(LOG_WARNING, "res_odbc: Error AllocHDB %d\n", res); obj->parent->last_negative_connect = ast_tvnow(); - ast_mutex_unlock(&obj->lock); return ODBC_FAIL; } - SQLSetConnectAttr(obj->con, SQL_LOGIN_TIMEOUT, (SQLPOINTER *)(long) obj->parent->conntimeout, 0); - SQLSetConnectAttr(obj->con, SQL_ATTR_CONNECTION_TIMEOUT, (SQLPOINTER *)(long) obj->parent->conntimeout, 0); + SQLSetConnectAttr(con, SQL_LOGIN_TIMEOUT, (SQLPOINTER *)(long) obj->parent->conntimeout, 0); + SQLSetConnectAttr(con, SQL_ATTR_CONNECTION_TIMEOUT, (SQLPOINTER *)(long) obj->parent->conntimeout, 0); #ifdef NEEDTRACE - SQLSetConnectAttr(obj->con, SQL_ATTR_TRACE, &enable, SQL_IS_INTEGER); - SQLSetConnectAttr(obj->con, SQL_ATTR_TRACEFILE, tracefile, strlen(tracefile)); + SQLSetConnectAttr(con, SQL_ATTR_TRACE, &enable, SQL_IS_INTEGER); + SQLSetConnectAttr(con, SQL_ATTR_TRACEFILE, tracefile, strlen(tracefile)); #endif - res = SQLConnect(obj->con, + res = SQLConnect(con, (SQLCHAR *) obj->parent->dsn, SQL_NTS, (SQLCHAR *) obj->parent->username, SQL_NTS, (SQLCHAR *) obj->parent->password, SQL_NTS); if ((res != SQL_SUCCESS) && (res != SQL_SUCCESS_WITH_INFO)) { - SQLGetDiagRec(SQL_HANDLE_DBC, obj->con, 1, state, &err, msg, 100, &mlen); + SQLGetDiagRec(SQL_HANDLE_DBC, con, 1, state, &err, msg, 100, &mlen); obj->parent->last_negative_connect = ast_tvnow(); - ast_mutex_unlock(&obj->lock); ast_log(LOG_WARNING, "res_odbc: Error SQLConnect=%d errno=%d %s\n", res, (int)err, msg); + if ((res = SQLFreeHandle(SQL_HANDLE_DBC, con) != SQL_SUCCESS)) { + SQLGetDiagRec(SQL_HANDLE_DBC, con, 1, state, &err, msg, 100, &mlen); + ast_log(LOG_WARNING, "Unable to deallocate database handle %p? %d errno=%d %s\n", obj->con, res, (int)err, msg); + } return ODBC_FAIL; } else { ast_log(LOG_NOTICE, "res_odbc: Connected to %s [%s]\n", obj->parent->name, obj->parent->dsn); @@ -1512,7 +1574,7 @@ static odbc_status odbc_obj_connect(struct odbc_ob obj->last_used = ast_tvnow(); } - ast_mutex_unlock(&obj->lock); + obj->con = con; return ODBC_SUCCESS; } @@ -1710,11 +1772,11 @@ static int data_odbc_provider_handler(const struct continue; } - ast_mutex_lock(¤t->lock); + ast_rwlock_wrlock(¤t->lock); ast_data_add_str(data_odbc_connection, "status", current->used ? "in use" : current->up && ast_odbc_sanity_check(current) ? "connected" : "disconnected"); ast_data_add_bool(data_odbc_connection, "transactional", current->tx); - ast_mutex_unlock(¤t->lock); + ast_rwlock_unlock(¤t->lock); if (class->haspool) { ast_data_add_int(data_odbc_connection, "number", ++count);