[Asterisk-code-review] Revert "pjproject-bundled: Add upstream timer fixes" (...asterisk[13])
Friendly Automation
asteriskteam at digium.com
Wed May 8 12:22:14 CDT 2019
Friendly Automation has submitted this change and it was merged. ( https://gerrit.asterisk.org/c/asterisk/+/11364 )
Change subject: Revert "pjproject-bundled: Add upstream timer fixes"
......................................................................
Revert "pjproject-bundled: Add upstream timer fixes"
This reverts commit cfeb8a59eb892e7f72b5ce0acfc0dbde1a51d3b8.
The fixes in question cause assert failures when pjproject
asserts are enabled. Reverting in 13 until a solution is
found for all branches.
Change-Id: Iae5bd340e0543613185fecb63f9c86fa985fe664
---
D third-party/pjproject/patches/0031-r2191-timer-fixes.patch
1 file changed, 0 insertions(+), 372 deletions(-)
Approvals:
Benjamin Keith Ford: Looks good to me, but someone else must approve
Joshua Colp: Looks good to me, approved
Friendly Automation: Approved for Submit
diff --git a/third-party/pjproject/patches/0031-r2191-timer-fixes.patch b/third-party/pjproject/patches/0031-r2191-timer-fixes.patch
deleted file mode 100644
index e1205f0..0000000
--- a/third-party/pjproject/patches/0031-r2191-timer-fixes.patch
+++ /dev/null
@@ -1,372 +0,0 @@
-From 27a076f2f6c6007c0ba41d2868a803c4d841e815 Mon Sep 17 00:00:00 2001
-From: nanang <nanang at localhost>
-Date: Tue, 23 Apr 2019 08:42:45 +0000
-Subject: [PATCH] Fixed #2191: - Stricter double timer entry scheduling
- prevention. - Integrate group lock in SIP transport, e.g: for add/dec ref,
- for timer scheduling.
-
----
- pjlib/include/pj/timer.h | 2 +-
- pjlib/src/pj/timer.c | 11 +++++++-
- pjsip/include/pjsip/sip_endpoint.h | 39 +++++++++++++++++++++++++++++
- pjsip/include/pjsip/sip_transport.h | 2 ++
- pjsip/src/pjsip/sip_endpoint.c | 36 ++++++++++++++++++++++++++
- pjsip/src/pjsip/sip_transport.c | 36 +++++++++++++++++++++-----
- pjsip/src/pjsip/sip_transport_tcp.c | 10 +++++---
- pjsip/src/pjsip/sip_transport_tls.c | 14 ++++++++---
- pjsip/src/pjsip/sip_transport_udp.c | 2 ++
- 9 files changed, 137 insertions(+), 15 deletions(-)
-
-diff --git a/pjlib/include/pj/timer.h b/pjlib/include/pj/timer.h
-index df6155a81..14857b872 100644
---- a/pjlib/include/pj/timer.h
-+++ b/pjlib/include/pj/timer.h
-@@ -252,9 +252,9 @@ PJ_DECL(pj_status_t) pj_timer_heap_schedule( pj_timer_heap_t *ht,
- *
- * @param ht The timer heap.
- * @param entry The entry to be registered.
-+ * @param delay The interval to expire.
- * @param id_val The value to be set to the "id" field of the timer entry
- * once the timer is scheduled.
-- * @param delay The interval to expire.
- * @param grp_lock The group lock.
- *
- * @return PJ_SUCCESS, or the appropriate error code.
-diff --git a/pjlib/src/pj/timer.c b/pjlib/src/pj/timer.c
-index f0a2cbbc9..cbdd9791f 100644
---- a/pjlib/src/pj/timer.c
-+++ b/pjlib/src/pj/timer.c
-@@ -502,7 +502,7 @@ static pj_status_t schedule_w_grp_lock(pj_timer_heap_t *ht,
- PJ_ASSERT_RETURN(entry->cb != NULL, PJ_EINVAL);
-
- /* Prevent same entry from being scheduled more than once */
-- PJ_ASSERT_RETURN(entry->_timer_id < 1, PJ_EINVALIDOP);
-+ //PJ_ASSERT_RETURN(entry->_timer_id < 1, PJ_EINVALIDOP);
-
- #if PJ_TIMER_DEBUG
- entry->src_file = src_file;
-@@ -512,6 +512,15 @@ static pj_status_t schedule_w_grp_lock(pj_timer_heap_t *ht,
- PJ_TIME_VAL_ADD(expires, *delay);
-
- lock_timer_heap(ht);
-+
-+ /* Prevent same entry from being scheduled more than once */
-+ if (pj_timer_entry_running(entry)) {
-+ unlock_timer_heap(ht);
-+ PJ_LOG(3,(THIS_FILE, "Bug! Rescheduling outstanding entry (%p)",
-+ entry));
-+ return PJ_EINVALIDOP;
-+ }
-+
- status = schedule_entry(ht, entry, &expires);
- if (status == PJ_SUCCESS) {
- if (set_id)
-diff --git a/pjsip/include/pjsip/sip_endpoint.h b/pjsip/include/pjsip/sip_endpoint.h
-index 99683fbe1..ee967f8d9 100644
---- a/pjsip/include/pjsip/sip_endpoint.h
-+++ b/pjsip/include/pjsip/sip_endpoint.h
-@@ -138,6 +138,7 @@ PJ_DECL(pj_status_t) pjsip_endpt_handle_events( pjsip_endpoint *endpt,
- PJ_DECL(pj_status_t) pjsip_endpt_handle_events2(pjsip_endpoint *endpt,
- const pj_time_val *max_timeout,
- unsigned *count);
-+
- /**
- * Schedule timer to endpoint's timer heap. Application must poll the endpoint
- * periodically (by calling #pjsip_endpt_handle_events) to ensure that the
-@@ -166,6 +167,44 @@ PJ_DECL(pj_status_t) pjsip_endpt_schedule_timer( pjsip_endpoint *endpt,
- const pj_time_val *delay );
- #endif
-
-+/**
-+ * Schedule timer to endpoint's timer heap with group lock. Application must
-+ * poll the endpoint periodically (by calling #pjsip_endpt_handle_events) to
-+ * ensure that the timer events are handled in timely manner. When the
-+ * timeout for the timer has elapsed, the callback specified in the entry
-+ * argument will be called. This function, like all other endpoint functions,
-+ * is thread safe.
-+ *
-+ * @param endpt The endpoint.
-+ * @param entry The timer entry.
-+ * @param delay The relative delay of the timer.
-+ * @param id_val The value to be set to the "id" field of the timer entry
-+ * once the timer is scheduled.
-+ * @param grp_lock The group lock.
-+ * @return PJ_OK (zero) if successfull.
-+ */
-+#if PJ_TIMER_DEBUG
-+#define pjsip_endpt_schedule_timer_w_grp_lock(ept,ent,d,id,gl) \
-+ pjsip_endpt_schedule_timer_w_grp_lock_dbg(ept,ent,d,id,gl,\
-+ __FILE__, __LINE__)
-+
-+PJ_DECL(pj_status_t) pjsip_endpt_schedule_timer_w_grp_lock_dbg(
-+ pjsip_endpoint *endpt,
-+ pj_timer_entry *entry,
-+ const pj_time_val *delay,
-+ int id_val,
-+ pj_grp_lock_t *grp_lock,
-+ const char *src_file,
-+ int src_line);
-+#else
-+PJ_DECL(pj_status_t) pjsip_endpt_schedule_timer_w_grp_lock(
-+ pjsip_endpoint *endpt,
-+ pj_timer_entry *entry,
-+ const pj_time_val *delay,
-+ int id_val,
-+ pj_grp_lock_t *grp_lock );
-+#endif
-+
- /**
- * Cancel the previously registered timer.
- * This function, like all other endpoint functions, is thread safe.
-diff --git a/pjsip/include/pjsip/sip_transport.h b/pjsip/include/pjsip/sip_transport.h
-index addc8d521..d1ff3618b 100644
---- a/pjsip/include/pjsip/sip_transport.h
-+++ b/pjsip/include/pjsip/sip_transport.h
-@@ -810,6 +810,8 @@ struct pjsip_transport
- pj_pool_t *pool; /**< Pool used by transport. */
- pj_atomic_t *ref_cnt; /**< Reference counter. */
- pj_lock_t *lock; /**< Lock object. */
-+ pj_grp_lock_t *grp_lock; /**< Group lock for sync with
-+ ioqueue and timer. */
- pj_bool_t tracing; /**< Tracing enabled? */
- pj_bool_t is_shutdown; /**< Being shutdown? */
- pj_bool_t is_destroying; /**< Destroy in progress? */
-diff --git a/pjsip/src/pjsip/sip_endpoint.c b/pjsip/src/pjsip/sip_endpoint.c
-index d810781d5..71bc761c2 100644
---- a/pjsip/src/pjsip/sip_endpoint.c
-+++ b/pjsip/src/pjsip/sip_endpoint.c
-@@ -802,6 +802,42 @@ PJ_DEF(pj_status_t) pjsip_endpt_schedule_timer( pjsip_endpoint *endpt,
- }
- #endif
-
-+/*
-+ * Schedule timer with group lock.
-+ */
-+#if PJ_TIMER_DEBUG
-+PJ_DEF(pj_status_t) pjsip_endpt_schedule_timer_w_grp_lock_dbg(
-+ pjsip_endpoint *endpt,
-+ pj_timer_entry *entry,
-+ const pj_time_val *delay,
-+ int id_val,
-+ pj_grp_lock_t *grp_lock,
-+ const char *src_file,
-+ int src_line)
-+{
-+ PJ_LOG(6, (THIS_FILE, "pjsip_endpt_schedule_timer_w_grp_lock"
-+ "(entry=%p, delay=%u.%u, grp_lock=%p)",
-+ entry, delay->sec, delay->msec, grp_lock));
-+ return pj_timer_heap_schedule_w_grp_lock_dbg(endpt->timer_heap, entry,
-+ delay, id_val, grp_lock,
-+ src_file, src_line);
-+}
-+#else
-+PJ_DEF(pj_status_t) pjsip_endpt_schedule_timer_w_grp_lock(
-+ pjsip_endpoint *endpt,
-+ pj_timer_entry *entry,
-+ const pj_time_val *delay,
-+ int id_val,
-+ pj_grp_lock_t *grp_lock )
-+{
-+ PJ_LOG(6, (THIS_FILE, "pjsip_endpt_schedule_timer_w_grp_lock"
-+ "(entry=%p, delay=%u.%u, grp_lock=%p)",
-+ entry, delay->sec, delay->msec, grp_lock));
-+ return pj_timer_heap_schedule_w_grp_lock( endpt->timer_heap, entry,
-+ delay, id_val, grp_lock );
-+}
-+#endif
-+
- /*
- * Cancel the previously registered timer.
- */
-diff --git a/pjsip/src/pjsip/sip_transport.c b/pjsip/src/pjsip/sip_transport.c
-index 67e235a39..529604399 100644
---- a/pjsip/src/pjsip/sip_transport.c
-+++ b/pjsip/src/pjsip/sip_transport.c
-@@ -1012,6 +1012,9 @@ static void transport_idle_callback(pj_timer_heap_t *timer_heap,
-
- PJ_UNUSED_ARG(timer_heap);
-
-+ if (entry->id == PJ_FALSE)
-+ return;
-+
- entry->id = PJ_FALSE;
- pjsip_transport_destroy(tp);
- }
-@@ -1049,6 +1052,10 @@ PJ_DEF(pj_status_t) pjsip_transport_add_ref( pjsip_transport *tp )
-
- PJ_ASSERT_RETURN(tp != NULL, PJ_EINVAL);
-
-+ /* Add ref transport group lock, if any */
-+ if (tp->grp_lock)
-+ pj_grp_lock_add_ref(tp->grp_lock);
-+
- /* Cache some vars for checking transport validity later */
- tpmgr = tp->tpmgr;
- key_len = sizeof(tp->key.type) + tp->addr_len;
-@@ -1063,8 +1070,8 @@ PJ_DEF(pj_status_t) pjsip_transport_add_ref( pjsip_transport *tp )
- pj_atomic_get(tp->ref_cnt) == 1)
- {
- if (tp->idle_timer.id != PJ_FALSE) {
-- pjsip_endpt_cancel_timer(tp->tpmgr->endpt, &tp->idle_timer);
- tp->idle_timer.id = PJ_FALSE;
-+ pjsip_endpt_cancel_timer(tp->tpmgr->endpt, &tp->idle_timer);
- }
- }
- pj_lock_release(tpmgr->lock);
-@@ -1114,14 +1121,23 @@ PJ_DEF(pj_status_t) pjsip_transport_dec_ref( pjsip_transport *tp )
- delay.msec = 0;
- }
-
-- pj_assert(tp->idle_timer.id == 0);
-- tp->idle_timer.id = PJ_TRUE;
-- pjsip_endpt_schedule_timer(tp->tpmgr->endpt, &tp->idle_timer,
-- &delay);
-+ /* Avoid double timer entry scheduling */
-+ if (pj_timer_entry_running(&tp->idle_timer))
-+ pjsip_endpt_cancel_timer(tp->tpmgr->endpt, &tp->idle_timer);
-+
-+ pjsip_endpt_schedule_timer_w_grp_lock(tp->tpmgr->endpt,
-+ &tp->idle_timer,
-+ &delay,
-+ PJ_TRUE,
-+ tp->grp_lock);
- }
- pj_lock_release(tpmgr->lock);
- }
-
-+ /* Dec ref transport group lock, if any */
-+ if (tp->grp_lock)
-+ pj_grp_lock_dec_ref(tp->grp_lock);
-+
- return PJ_SUCCESS;
- }
-
-@@ -1168,6 +1184,10 @@ PJ_DEF(pj_status_t) pjsip_transport_register( pjsip_tpmgr *mgr,
- /* Register new entry */
- pj_hash_set(tp->pool, mgr->table, &tp->key, key_len, hval, tp);
-
-+ /* Add ref transport group lock, if any */
-+ if (tp->grp_lock)
-+ pj_grp_lock_add_ref(tp->grp_lock);
-+
- pj_lock_release(mgr->lock);
-
- TRACE_((THIS_FILE,"Transport %s registered: type=%s, remote=%s:%d",
-@@ -1199,8 +1219,8 @@ static pj_status_t destroy_transport( pjsip_tpmgr *mgr,
- */
- //pj_assert(tp->idle_timer.id == PJ_FALSE);
- if (tp->idle_timer.id != PJ_FALSE) {
-- pjsip_endpt_cancel_timer(mgr->endpt, &tp->idle_timer);
- tp->idle_timer.id = PJ_FALSE;
-+ pjsip_endpt_cancel_timer(mgr->endpt, &tp->idle_timer);
- }
-
- /*
-@@ -1226,6 +1246,10 @@ static pj_status_t destroy_transport( pjsip_tpmgr *mgr,
- pj_lock_release(mgr->lock);
- pj_lock_release(tp->lock);
-
-+ /* Dec ref transport group lock, if any */
-+ if (tp->grp_lock)
-+ pj_grp_lock_dec_ref(tp->grp_lock);
-+
- /* Destroy. */
- return tp->destroy(tp);
- }
-diff --git a/pjsip/src/pjsip/sip_transport_tcp.c b/pjsip/src/pjsip/sip_transport_tcp.c
-index fe327459e..374bf461b 100644
---- a/pjsip/src/pjsip/sip_transport_tcp.c
-+++ b/pjsip/src/pjsip/sip_transport_tcp.c
-@@ -692,6 +692,8 @@ static pj_status_t tcp_create( struct tcp_listener *listener,
- pj_grp_lock_add_ref(tcp->grp_lock);
- pj_grp_lock_add_handler(tcp->grp_lock, pool, tcp, &tcp_on_destroy);
-
-+ tcp->base.grp_lock = tcp->grp_lock;
-+
- /* Create active socket */
- pj_activesock_cfg_default(&asock_cfg);
- asock_cfg.async_cnt = 1;
-@@ -746,7 +748,11 @@ static pj_status_t tcp_create( struct tcp_listener *listener,
- return PJ_SUCCESS;
-
- on_error:
-- tcp_destroy(&tcp->base, status);
-+ if (tcp->grp_lock && pj_grp_lock_get_ref(tcp->grp_lock))
-+ tcp_destroy(&tcp->base, status);
-+ else
-+ tcp_on_destroy(tcp);
-+
- return status;
- }
-
-@@ -867,8 +873,6 @@ static pj_status_t tcp_destroy(pjsip_transport *transport,
- tcp->grp_lock = NULL;
- pj_grp_lock_dec_ref(grp_lock);
- /* Transport may have been deleted at this point */
-- } else {
-- tcp_on_destroy(tcp);
- }
-
- return PJ_SUCCESS;
-diff --git a/pjsip/src/pjsip/sip_transport_tls.c b/pjsip/src/pjsip/sip_transport_tls.c
-index d3afae5e9..dd3a4d639 100644
---- a/pjsip/src/pjsip/sip_transport_tls.c
-+++ b/pjsip/src/pjsip/sip_transport_tls.c
-@@ -165,6 +165,10 @@ static pj_status_t tls_create(struct tls_listener *listener,
- struct tls_transport **p_tls);
-
-
-+/* Clean up TLS resources */
-+static void tls_on_destroy(void *arg);
-+
-+
- static void tls_perror(const char *sender, const char *title,
- pj_status_t status)
- {
-@@ -893,7 +897,11 @@ static pj_status_t tls_create( struct tls_listener *listener,
- return PJ_SUCCESS;
-
- on_error:
-- tls_destroy(&tls->base, status);
-+ if (tls->grp_lock && pj_grp_lock_get_ref(tls->grp_lock))
-+ tls_destroy(&tls->base, status);
-+ else
-+ tls_on_destroy(tls);
-+
- return status;
- }
-
-@@ -1048,8 +1056,6 @@ static pj_status_t tls_destroy(pjsip_transport *transport,
- tls->grp_lock = NULL;
- pj_grp_lock_dec_ref(grp_lock);
- /* Transport may have been deleted at this point */
-- } else {
-- tls_on_destroy(tls);
- }
-
- return PJ_SUCCESS;
-@@ -1235,7 +1241,7 @@ static pj_status_t lis_create_transport(pjsip_tpfactory *factory,
- pj_ssl_sock_set_user_data(tls->ssock, tls);
-
- /* Set up the group lock */
-- tls->grp_lock = glock;
-+ tls->grp_lock = tls->base.grp_lock = glock;
- pj_grp_lock_add_ref(tls->grp_lock);
- pj_grp_lock_add_handler(tls->grp_lock, pool, tls, &tls_on_destroy);
-
-diff --git a/pjsip/src/pjsip/sip_transport_udp.c b/pjsip/src/pjsip/sip_transport_udp.c
-index dbda474cf..b82d519c9 100644
---- a/pjsip/src/pjsip/sip_transport_udp.c
-+++ b/pjsip/src/pjsip/sip_transport_udp.c
-@@ -691,6 +691,8 @@ static pj_status_t register_to_ioqueue(struct udp_transport *tp)
- pj_grp_lock_add_ref(tp->grp_lock);
- pj_grp_lock_add_handler(tp->grp_lock, tp->base.pool, tp,
- &udp_on_destroy);
-+
-+ tp->base.grp_lock = tp->grp_lock;
- }
-
- /* Register to ioqueue. */
---
-2.20.1
-
--
To view, visit https://gerrit.asterisk.org/c/asterisk/+/11364
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings
Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-Change-Id: Iae5bd340e0543613185fecb63f9c86fa985fe664
Gerrit-Change-Number: 11364
Gerrit-PatchSet: 1
Gerrit-Owner: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Benjamin Keith Ford <bford at digium.com>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-Reviewer: Sean Bright <sean.bright at gmail.com>
Gerrit-MessageType: merged
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20190508/69f355be/attachment-0001.html>
More information about the asterisk-code-review
mailing list