From 20f672539ee00fef7205b12d8ed572421f941bd7 Mon Sep 17 00:00:00 2001 From: Sean Bright Date: Fri, 18 Jan 2019 17:11:18 -0500 Subject: [PATCH] pjsip_transport_management: Shutdown transport immediately on disconnect The transport management code that checks for idle connections keeps a reference to PJSIP's transport for IDLE_TIMEOUT milliseconds (32000 by default). Because of this, if the transport is closed before this timeout, the idle checking code will keep the transport from actually being shutdown until the timeout expires. Rather than passing the AO2 object to the scheduler task, we just pass its key and look it up when it is time to potentially close the idle connection. The other transport management code handles cleaning up everything else for us. Additionally, because we use the address of the transport when generating its name, we concatenate an incrementing ID to the end of the name to guarantee uniqueness. Related to ASTERISK~28231 Change-Id: I02ee9f4073b6abca9169d30c47aa69b5e8ae9afb --- res/res_pjsip/pjsip_transport_management.c | 77 ++++++++++++++-------- res/res_pjsip_transport_websocket.c | 9 ++- 2 files changed, 55 insertions(+), 31 deletions(-) diff --git a/res/res_pjsip/pjsip_transport_management.c b/res/res_pjsip/pjsip_transport_management.c index a3cfde9920..af572d3b6f 100644 --- a/res/res_pjsip/pjsip_transport_management.c +++ b/res/res_pjsip/pjsip_transport_management.c @@ -139,34 +139,62 @@ static int idle_sched_init_pj_thread(void) return 0; } +static struct monitored_transport *get_monitored_transport_by_name(const char *obj_name) +{ + struct ao2_container *transports; + struct monitored_transport *monitored = NULL; + + transports = ao2_global_obj_ref(monitored_transports); + if (transports) { + monitored = ao2_find(transports, obj_name, OBJ_SEARCH_KEY); + } + ao2_cleanup(transports); + + /* Caller is responsible for cleaning up reference */ + return monitored; +} + static int idle_sched_cb(const void *data) { - struct monitored_transport *monitored = (struct monitored_transport *) data; + char *obj_name = (char *) data; + struct monitored_transport *monitored; if (idle_sched_init_pj_thread()) { - ao2_ref(monitored, -1); + ast_free(obj_name); return 0; } - if (!monitored->sip_received) { - ast_log(LOG_NOTICE, "Shutting down transport '%s' since no request was received in %d seconds\n", - monitored->transport->info, IDLE_TIMEOUT / 1000); - pjsip_transport_shutdown(monitored->transport); + monitored = get_monitored_transport_by_name(obj_name); + if (monitored) { + if (!monitored->sip_received) { + ast_log(LOG_NOTICE, "Shutting down transport '%s' since no request was received in %d seconds\n", + monitored->transport->info, IDLE_TIMEOUT / 1000); + pjsip_transport_shutdown(monitored->transport); + } + ao2_ref(monitored, -1); } - ao2_ref(monitored, -1); + ast_free(obj_name); return 0; } static int idle_sched_cleanup(const void *data) { - struct monitored_transport *monitored = (struct monitored_transport *) data; + char *obj_name = (char *) data; + struct monitored_transport *monitored; - if (!idle_sched_init_pj_thread()) { + if (idle_sched_init_pj_thread()) { + ast_free(obj_name); + return 0; + } + + monitored = get_monitored_transport_by_name(obj_name); + if (monitored) { pjsip_transport_shutdown(monitored->transport); + ao2_ref(monitored, -1); } - ao2_ref(monitored, -1); + ast_free(obj_name); return 0; } @@ -203,13 +231,13 @@ static void monitored_transport_state_callback(pjsip_transport *transport, pjsip ao2_link(transports, monitored); if (transport->dir == PJSIP_TP_DIR_INCOMING) { - /* Let the scheduler inherit the reference from allocation */ - if (ast_sched_add_variable(sched, IDLE_TIMEOUT, idle_sched_cb, monitored, 1) < 0) { - /* Uh Oh. Could not schedule the idle check. Kill the transport. */ + char *obj_name = ast_strdup(transport->obj_name); + + if (!obj_name + || ast_sched_add_variable(sched, IDLE_TIMEOUT, idle_sched_cb, obj_name, 1) < 0) { + /* Shut down the transport if anything fails */ pjsip_transport_shutdown(transport); - } else { - /* monitored ref successfully passed to idle_sched_cb() */ - break; + ast_free(obj_name); } } ao2_ref(monitored, -1); @@ -324,23 +352,14 @@ static struct ast_sorcery_observer keepalive_global_observer = { */ static pj_bool_t idle_monitor_on_rx_request(pjsip_rx_data *rdata) { - struct ao2_container *transports; struct monitored_transport *idle_trans; - transports = ao2_global_obj_ref(monitored_transports); - if (!transports) { - return PJ_FALSE; - } - - idle_trans = ao2_find(transports, rdata->tp_info.transport->obj_name, OBJ_SEARCH_KEY); - ao2_ref(transports, -1); - if (!idle_trans) { - return PJ_FALSE; + idle_trans = get_monitored_transport_by_name(rdata->tp_info.transport->obj_name); + if (idle_trans) { + idle_trans->sip_received = 1; + ao2_ref(idle_trans, -1); } - idle_trans->sip_received = 1; - ao2_ref(idle_trans, -1); - return PJ_FALSE; } diff --git a/res/res_pjsip_transport_websocket.c b/res/res_pjsip_transport_websocket.c index 73268eac91..8bc078b02e 100644 --- a/res/res_pjsip_transport_websocket.c +++ b/res/res_pjsip_transport_websocket.c @@ -41,6 +41,11 @@ static int transport_type_wss; static int transport_type_wss_ipv6; +/*! + * Used to ensure uniqueness among WS transport names + */ +static int ws_obj_name_serial; + /*! * \brief Wrapper for pjsip_transport, for storing the WebSocket session */ @@ -163,8 +168,8 @@ static int transport_create(void *data) } /* Give websocket transport a unique name for its lifetime */ - snprintf(newtransport->transport.obj_name, PJ_MAX_OBJ_NAME, "ws%p", - &newtransport->transport); + snprintf(newtransport->transport.obj_name, PJ_MAX_OBJ_NAME, "ws%p-%d", + &newtransport->transport, ast_atomic_fetchadd_int(&ws_obj_name_serial, 1)); newtransport->transport.endpt = endpt;