From 6eb7a17503ac013145f44248fc090d6dde5ca9de Mon Sep 17 00:00:00 2001 From: Mohamed Chibani Date: Thu, 28 Oct 2021 09:53:33 -0400 Subject: [PATCH] ICE - rework TCP reconnection Introduce two main changes to the TCP patch: - limit the number of connection retries - use a higher Ta for TCP connection retry Gitlab: #634 Change-Id: Ifbddcceb2c4d15d6c2bed6c62ae8cf5c535892cf --- pjlib/src/pj/ioqueue_common_abs.h | 11 --- pjnath/include/pjnath/ice_session.h | 9 ++- pjnath/src/pjnath/ice_session.c | 110 +++++++++++++++++++--------- pjnath/src/pjnath/ice_strans.c | 2 +- pjsip/src/pjsip/sip_transport.c | 18 ----- 5 files changed, 84 insertions(+), 66 deletions(-) diff --git a/pjlib/src/pj/ioqueue_common_abs.h b/pjlib/src/pj/ioqueue_common_abs.h index 14201848d..29fd14f39 100644 --- a/pjlib/src/pj/ioqueue_common_abs.h +++ b/pjlib/src/pj/ioqueue_common_abs.h @@ -89,19 +89,11 @@ union operation_key }; #if PJ_IOQUEUE_HAS_SAFE_UNREG -<<<<<<< HEAD # define UNREG_FIELDS \ unsigned ref_count; \ pj_bool_t closing; \ pj_time_val free_time; \ -======= -# define UNREG_FIELDS \ - unsigned ref_count; \ - pj_bool_t closing; \ - pj_time_val free_time; \ - ->>>>>>> 26a1072f6 (0008-fix_ioqueue_ipv6_sendto) #else # define UNREG_FIELDS #endif @@ -148,10 +140,7 @@ static void ioqueue_add_to_set2(pj_ioqueue_t *ioqueue, static void ioqueue_remove_from_set( pj_ioqueue_t *ioqueue, pj_ioqueue_key_t *key, enum ioqueue_event_type event_type); -<<<<<<< HEAD static void ioqueue_remove_from_set2(pj_ioqueue_t *ioqueue, pj_ioqueue_key_t *key, unsigned event_types); -======= ->>>>>>> 26a1072f6 (0008-fix_ioqueue_ipv6_sendto) diff --git a/pjnath/include/pjnath/ice_session.h b/pjnath/include/pjnath/ice_session.h index 760540ea7..808224783 100644 --- a/pjnath/include/pjnath/ice_session.h +++ b/pjnath/include/pjnath/ice_session.h @@ -485,7 +485,14 @@ struct pj_ice_sess_check * When the check failed, this will contain the failure status of the * STUN transaction. */ - pj_status_t err_code; + pj_status_t err_code; + +#if PJ_HAS_TCP + /** + * TCP reconnection attemps counter. + */ + int reconnect_count; +#endif }; diff --git a/pjnath/src/pjnath/ice_session.c b/pjnath/src/pjnath/ice_session.c index e452bf9f7..bc1e3f715 100644 --- a/pjnath/src/pjnath/ice_session.c +++ b/pjnath/src/pjnath/ice_session.c @@ -2278,6 +2278,9 @@ static pj_status_t add_rcand_and_update_checklist( chk->state = PJ_ICE_SESS_CHECK_STATE_FROZEN; chk->foundation_idx = get_check_foundation_idx(ice, lcand, rcand, PJ_TRUE); +#if PJ_HAS_TCP + chk->reconnect_count = 0; +#endif /* Check if foundation cannot be added (e.g: list is full) */ if (chk->foundation_idx < 0) @@ -2679,9 +2682,13 @@ static pj_status_t perform_check(pj_ice_sess *ice, if (ice->timer_connect.id != TIMER_NONE) { pj_assert(!"Not expected any timer active"); } else { + LOG5((ice->obj_name, + "Scheduling connection time-out for check %s", + dump_check(ice->tmp.txt, sizeof(ice->tmp.txt), clist, check))); + pj_time_val delay = { - .sec = 15, - .msec = 0, + .sec = 0, + .msec = PJ_ICE_TCP_CONNECTION_TIMEOUT, }; pj_time_val_normalize(&delay); pj_timer_heap_schedule_w_grp_lock(ice->stun_cfg.timer_heap, @@ -2735,6 +2742,8 @@ static pj_status_t start_periodic_check(pj_timer_heap_t *th, td = (struct timer_data*) te->user_data; ice = td->ice; clist = td->clist; + // Default Ta timer + pj_time_val timeout = {0, PJ_ICE_TA_VAL}; pj_grp_lock_acquire(ice->grp_lock); @@ -2751,6 +2760,31 @@ static pj_status_t start_periodic_check(pj_timer_heap_t *th, pj_ice_sess_check *check = NULL; + /* Send STUN Binding request for check with highest priority on + * Waiting state. + */ + + if (start_count == 0) { + for (i = 0; i < clist->count; ++i) { + check = &clist->checks[i]; + + if (check->state == PJ_ICE_SESS_CHECK_STATE_WAITING) { + LOG5((ice->obj_name, "Starting periodic check for check %i (was waiting)", i)); + pj_log_push_indent(); + + status = perform_check(ice, clist, i, ice->is_nominating); + if (status != PJ_SUCCESS && status != PJ_EPENDING) { + check_set_state(ice, check, PJ_ICE_SESS_CHECK_STATE_FAILED, + status); + on_check_complete(ice, check); + } + ++start_count; + break; + } + } + } + +#if PJ_HAS_TCP /* Send STUN Binding request for check with highest priority on * Retry state. */ @@ -2768,6 +2802,9 @@ static pj_status_t start_periodic_check(pj_timer_heap_t *th, status); on_check_complete(ice, check); } + timeout.msec = PJ_ICE_TCP_RECONNECTION_DELAY; + timeout.sec = 0; + ++start_count; break; } @@ -2799,30 +2836,7 @@ static pj_status_t start_periodic_check(pj_timer_heap_t *th, } } } - - /* Send STUN Binding request for check with highest priority on - * Waiting state. - */ - - if (start_count == 0) { - for (i = 0; i < clist->count; ++i) { - check = &clist->checks[i]; - - if (check->state == PJ_ICE_SESS_CHECK_STATE_WAITING) { - LOG5((ice->obj_name, "Starting periodic check for check %i (was waiting)", i)); - pj_log_push_indent(); - - status = perform_check(ice, clist, i, ice->is_nominating); - if (status != PJ_SUCCESS && status != PJ_EPENDING) { - check_set_state(ice, check, PJ_ICE_SESS_CHECK_STATE_FAILED, - status); - on_check_complete(ice, check); - } - ++start_count; - break; - } - } - } +#endif /* If we don't have anything in Waiting state, perform check to * highest priority pair that is in Frozen state. @@ -2845,6 +2859,7 @@ static pj_status_t start_periodic_check(pj_timer_heap_t *th, } } +#if PJ_HAS_TCP if (start_count == 0) { // If all sockets are pending, do nothing for (i = 0; i < clist->count; ++i) { @@ -2855,13 +2870,12 @@ static pj_status_t start_periodic_check(pj_timer_heap_t *th, } } } +#endif // The checks are performed at the rate of 1 check per Ta // interval. If a new check was started, we need to re-schedule // for the next one (if any). if (start_count!=0) { - pj_time_val timeout = {0, PJ_ICE_TA_VAL}; - pj_time_val_normalize(&timeout); pj_timer_heap_schedule_w_grp_lock(th, te, &timeout, PJ_TRUE, ice->grp_lock); @@ -3172,12 +3186,22 @@ void ice_sess_on_peer_connection(pj_ice_sess *ice, * the relayed candidate. This is done by set_perm from the other case. * But from this side, we can't know if the peer has authorized us. If it's * not the case, the connection will got a CONNECTION RESET BY PEER status. - * In this case, we can try to reconnect a bit after and this until the check - * reached its timeout. + * In this case, we try to reconnect few times with a delay between two + * attempts. */ - check->state = PJ_ICE_SESS_CHECK_STATE_NEEDS_RETRY; - check_set_state(ice, check,PJ_ICE_SESS_CHECK_STATE_NEEDS_RETRY, - status); + if (check->reconnect_count < PJ_ICE_TCP_MAX_RECONNECTION_COUNT) { + check->state = PJ_ICE_SESS_CHECK_STATE_NEEDS_RETRY; + check_set_state(ice, check,PJ_ICE_SESS_CHECK_STATE_NEEDS_RETRY, + status); + check->reconnect_count++; + } else { + // Max attempts reached. Fail this check. + LOG4((ice->obj_name, "Check %s: connection failed after %d attempts", + dump_check(ice->tmp.txt, sizeof(ice->tmp.txt), &ice->clist, check), + PJ_ICE_TCP_MAX_RECONNECTION_COUNT)); + check_set_state(ice, check, PJ_ICE_SESS_CHECK_STATE_FAILED, status); + on_check_complete(ice, check); + } pj_grp_lock_release(ice->grp_lock); return; } else if (status != PJ_SUCCESS) { @@ -3235,8 +3259,21 @@ void ice_sess_on_peer_connection(pj_ice_sess *ice, * In this case, we can try to reconnect a bit after and this until the check * reached its timeout. */ - check_set_state(ice, check,PJ_ICE_SESS_CHECK_STATE_NEEDS_RETRY, - status); + + if (check->reconnect_count < PJ_ICE_TCP_MAX_RECONNECTION_COUNT) { + check_set_state(ice, check, PJ_ICE_SESS_CHECK_STATE_NEEDS_RETRY, + status); + check->reconnect_count++; + } else { + // Max attempts reached. Fail this check. + LOG4((ice->obj_name, "Check %s: connection failed after %d attempts", + dump_check(ice->tmp.txt, sizeof(ice->tmp.txt), &ice->clist, check), + PJ_ICE_TCP_MAX_RECONNECTION_COUNT)); + check_set_state(ice, check, PJ_ICE_SESS_CHECK_STATE_FAILED, status); + on_check_complete(ice, check); + pj_grp_lock_release(ice->grp_lock); + return; + } } else if (status == PJ_EBUSY /* EBUSY */) { check_set_state(ice, check, PJ_ICE_SESS_CHECK_STATE_NEEDS_FIRST_PACKET, status); @@ -3682,6 +3719,9 @@ static void on_stun_request_complete(pj_stun_session *stun_sess, new_check->state = PJ_ICE_SESS_CHECK_STATE_SUCCEEDED; new_check->nominated = check->nominated; new_check->err_code = PJ_SUCCESS; +#if PJ_HAS_TCP + new_check->reconnect_count = 0; +#endif } else { new_check = &ice->valid_list.checks[i]; ice->valid_list.checks[i].nominated = check->nominated; diff --git a/pjnath/src/pjnath/ice_strans.c b/pjnath/src/pjnath/ice_strans.c index ee134e353..bfdf7fb74 100644 --- a/pjnath/src/pjnath/ice_strans.c +++ b/pjnath/src/pjnath/ice_strans.c @@ -891,7 +891,7 @@ static pj_bool_t add_local_candidate(pj_ice_sess_cand *cand, return PJ_SUCCESS; } else if (stun_cfg->af == pj_AF_INET6()) { - pj_in6_addr in6addr = {{0}}; + pj_in6_addr in6addr = {0}; in6addr.s6_addr[15] = 1; if (pj_memcmp(&in6addr, &addr->ipv6.sin6_addr, sizeof(in6addr))==0) diff --git a/pjsip/src/pjsip/sip_transport.c b/pjsip/src/pjsip/sip_transport.c index b24a91ba5..365e0a6f0 100644 --- a/pjsip/src/pjsip/sip_transport.c +++ b/pjsip/src/pjsip/sip_transport.c @@ -2480,7 +2480,6 @@ PJ_DEF(pj_status_t) pjsip_tpmgr_acquire_transport2(pjsip_tpmgr *mgr, } else { -<<<<<<< HEAD /* Find factory with type matches the destination type */ factory = mgr->factory_list.next; while (factory != &mgr->factory_list) { @@ -2488,23 +2487,6 @@ PJ_DEF(pj_status_t) pjsip_tpmgr_acquire_transport2(pjsip_tpmgr *mgr, break; factory = factory->next; } -======= - /* Make sure we don't use another factory than the one given if - secure flag is set */ - if (flag & PJSIP_TRANSPORT_SECURE) { - TRACE_((THIS_FILE, "Can't create new TLS transport with no " - "provided suitable TLS listener.")); - return PJSIP_ETPNOTSUITABLE; - } - - /* Find factory with type matches the destination type */ - factory = mgr->factory_list.next; - while (factory != &mgr->factory_list) { - if (factory->type == type) - break; - factory = factory->next; - } ->>>>>>> e6a22f11e (0004 multiple listeners) if (factory == &mgr->factory_list) { /* No factory can create the transport! */