[master] 51e6ded88 Use a separate condvar for connection-level flow control updates

[Dag Haavi Finstad]

commit 51e6ded883b7655091bd6a8ffb8869fa7d40b071
Author: Dag Haavi Finstad <daghf at varnish-software.com>
Date:   Wed Jun 27 13:27:34 2018 +0200

    Use a separate condvar for connection-level flow control updates
    
    The current flow control code's use of h2->cond is racy.
    
    h2->cond is already used for handing over a DATA frame to a stream
    thread. In the event that we have both streams waiting on this condvar
    for window updates and at the same time the rxthread gets signaled for a
    DATA frame, we could end up waking up the wrong thread and the rxthread
    gets stuck forever.
    
    This commit addresses this by using a separate condvar for window
    updates.
    
    An alternative would be to always issue a broadcast on h2->cond instead
    of signal, but I found this approach much cleaner.
    
    Probably fixes: #2623

diff --git a/bin/varnishd/http2/cache_http2.h b/bin/varnishd/http2/cache_http2.h
index a1d6f849b..fbc190535 100644
--- a/bin/varnishd/http2/cache_http2.h
+++ b/bin/varnishd/http2/cache_http2.h
@@ -145,6 +145,7 @@ struct h2_sess {
 	pthread_t			rxthr;
 	struct h2_req			*mailcall;
 	pthread_cond_t			*cond;
+	pthread_cond_t			winupd_cond[1];
 
 	struct sess			*sess;
 	int				refcnt;
diff --git a/bin/varnishd/http2/cache_http2_proto.c b/bin/varnishd/http2/cache_http2_proto.c
index 9dede792e..e5106e8a4 100644
--- a/bin/varnishd/http2/cache_http2_proto.c
+++ b/bin/varnishd/http2/cache_http2_proto.c
@@ -369,7 +369,7 @@ h2_rx_window_update(struct worker *wrk, struct h2_sess *h2, struct h2_req *r2)
 	Lck_Lock(&h2->sess->mtx);
 	r2->t_window += wu;
 	if (r2 == h2->req0)
-		AZ(pthread_cond_broadcast(h2->cond));
+		AZ(pthread_cond_broadcast(h2->winupd_cond));
 	else if (r2->cond != NULL)
 		AZ(pthread_cond_signal(r2->cond));
 	Lck_Unlock(&h2->sess->mtx);
diff --git a/bin/varnishd/http2/cache_http2_send.c b/bin/varnishd/http2/cache_http2_send.c
index cfbb51be7..7c290bf9d 100644
--- a/bin/varnishd/http2/cache_http2_send.c
+++ b/bin/varnishd/http2/cache_http2_send.c
@@ -213,7 +213,7 @@ h2_do_window(struct worker *wrk, struct h2_req *r2,
 			r2->cond = NULL;
 		}
 		while (h2->req0->t_window <= 0 && h2_errcheck(r2, h2) == 0) {
-			AZ(Lck_CondWait(h2->cond, &h2->sess->mtx, 0));
+			AZ(Lck_CondWait(h2->winupd_cond, &h2->sess->mtx, 0));
 		}
 
 		if (h2_errcheck(r2, h2) == 0) {
diff --git a/bin/varnishd/http2/cache_http2_session.c b/bin/varnishd/http2/cache_http2_session.c
index b350f05c3..c0545265c 100644
--- a/bin/varnishd/http2/cache_http2_session.c
+++ b/bin/varnishd/http2/cache_http2_session.c
@@ -123,6 +123,7 @@ h2_init_sess(const struct worker *wrk, struct sess *sp,
 		h2->htc->rfd = &sp->fd;
 		h2->sess = sp;
 		h2->rxthr = pthread_self();
+		AZ(pthread_cond_init(h2->winupd_cond, NULL));
 		VTAILQ_INIT(&h2->streams);
 		VTAILQ_INIT(&h2->txqueue);
 		h2_local_settings(&h2->local_settings);
@@ -150,6 +151,7 @@ h2_del_sess(struct worker *wrk, struct h2_sess *h2, enum sess_close reason)
 	assert(VTAILQ_EMPTY(&h2->streams));
 
 	VHT_Fini(h2->dectbl);
+	AZ(pthread_cond_destroy(h2->winupd_cond));
 	req = h2->srq;
 	AZ(req->ws->r);
 	sp = h2->sess;
@@ -404,7 +406,7 @@ h2_new_session(struct worker *wrk, void *arg)
 		if (r2->cond != NULL)
 			AZ(pthread_cond_signal(r2->cond));
 	}
-	AZ(pthread_cond_broadcast(h2->cond));
+	AZ(pthread_cond_broadcast(h2->winupd_cond));
 	Lck_Unlock(&h2->sess->mtx);
 	while (1) {
 		again = 0;