Ticket #460: Concurrency problem when destroying stream (thanks Michael Broughton)

git-svn-id: http://svn.pjsip.org/repos/pjproject/trunk@1790 74dad513-b988-da41-8d7b-12977e46ad98

3 files changed, 35 insertions, 6 deletions

diff --git a/pjmedia/src/pjmedia/stream.c b/pjmedia/src/pjmedia/stream.c
index 05681520..d4496612 100644
--- a/pjmedia/src/pjmedia/stream.c
+++ b/pjmedia/src/pjmedia/stream.c
@@ -1399,17 +1399,20 @@ PJ_DEF(pj_status_t) pjmedia_stream_destroy( pjmedia_stream *stream )
     PJ_ASSERT_RETURN(stream != NULL, PJ_EINVAL);
 
 
+    /* Detach from transport 
+     * MUST NOT hold stream mutex while detaching from transport, as
+     * it may cause deadlock. See ticket #460 for the details.
+     */
+    if (stream->transport) {
+	pjmedia_transport_detach(stream->transport, stream);
+	stream->transport = NULL;
+    }
+
     /* This function may be called when stream is partly initialized. */
     if (stream->jb_mutex)
 	pj_mutex_lock(stream->jb_mutex);
 
 
-    /* Detach from transport */
-    if (stream->transport) {
-	(*stream->transport->op->detach)(stream->transport, stream);
-	stream->transport = NULL;
-    }
-
     /* Free codec. */
 
     if (stream->codec) {
diff --git a/pjmedia/src/pjmedia/transport_ice.c b/pjmedia/src/pjmedia/transport_ice.c
index ed7efbc8..6c3b1dcf 100644
--- a/pjmedia/src/pjmedia/transport_ice.c
+++ b/pjmedia/src/pjmedia/transport_ice.c
@@ -698,6 +698,8 @@ static void transport_detach(pjmedia_transport *tp,
 {
     struct transport_ice *tp_ice = (struct transport_ice*)tp;
 
+    /* TODO: need to solve ticket #460 here */
+
     tp_ice->rtp_cb = NULL;
     tp_ice->rtcp_cb = NULL;
     tp_ice->stream = NULL;
diff --git a/pjmedia/src/pjmedia/transport_udp.c b/pjmedia/src/pjmedia/transport_udp.c
index faaf7674..3e56ea56 100644
--- a/pjmedia/src/pjmedia/transport_udp.c
+++ b/pjmedia/src/pjmedia/transport_udp.c
@@ -319,6 +319,13 @@ PJ_DEF(pj_status_t) pjmedia_transport_udp_attach( pjmedia_endpt *endpt,
     if (status != PJ_SUCCESS)
 	goto on_error;
     
+    /* Disallow concurrency so that detach() and destroy() are
+     * synchronized with the callback.
+     */
+    status = pj_ioqueue_set_concurrency(tp->rtp_key, PJ_FALSE);
+    if (status != PJ_SUCCESS)
+	goto on_error;
+
     pj_ioqueue_op_key_init(&tp->rtp_read_op, sizeof(tp->rtp_read_op));
     for (i=0; i<PJ_ARRAY_SIZE(tp->rtp_pending_write); ++i)
 	pj_ioqueue_op_key_init(&tp->rtp_pending_write[i].op_key, 
@@ -343,6 +350,10 @@ PJ_DEF(pj_status_t) pjmedia_transport_udp_attach( pjmedia_endpt *endpt,
     if (status != PJ_SUCCESS)
 	goto on_error;
 
+    status = pj_ioqueue_set_concurrency(tp->rtcp_key, PJ_FALSE);
+    if (status != PJ_SUCCESS)
+	goto on_error;
+
     pj_ioqueue_op_key_init(&tp->rtcp_read_op, sizeof(tp->rtcp_read_op));
     pj_ioqueue_op_key_init(&tp->rtcp_write_op, sizeof(tp->rtcp_write_op));
 
@@ -383,6 +394,9 @@ static pj_status_t transport_destroy(pjmedia_transport *tp)
     
 
     if (udp->rtp_key) {
+	/* This will block the execution if callback is still
+	 * being called.
+	 */
 	pj_ioqueue_unregister(udp->rtp_key);
 	udp->rtp_key = NULL;
 	udp->rtp_sock = PJ_INVALID_SOCKET;
@@ -645,6 +659,12 @@ static void transport_detach( pjmedia_transport *tp,
     pj_assert(tp);
 
     if (udp->attached) {
+	/* Lock the ioqueue keys to make sure that callbacks are
+	 * not executed. See ticket #460 for details.
+	 */
+	pj_ioqueue_lock_key(udp->rtp_key);
+	pj_ioqueue_lock_key(udp->rtcp_key);
+
 	/* User data is unreferenced on Release build */
 	PJ_UNUSED_ARG(user_data);
 
@@ -658,6 +678,10 @@ static void transport_detach( pjmedia_transport *tp,
 	udp->rtp_cb = NULL;
 	udp->rtcp_cb = NULL;
 	udp->user_data = NULL;
+
+	/* Unlock keys */
+	pj_ioqueue_unlock_key(udp->rtcp_key);
+	pj_ioqueue_unlock_key(udp->rtp_key);
     }
 }