Commit a6533c74 authored by Dag Haavi Finstad's avatar Dag Haavi Finstad Committed by Dridi Boukelmoune

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
parent aa809b5d
...@@ -145,6 +145,7 @@ struct h2_sess { ...@@ -145,6 +145,7 @@ struct h2_sess {
pthread_t rxthr; pthread_t rxthr;
struct h2_req *mailcall; struct h2_req *mailcall;
pthread_cond_t *cond; pthread_cond_t *cond;
pthread_cond_t winupd_cond[1];
struct sess *sess; struct sess *sess;
int refcnt; int refcnt;
......
...@@ -369,7 +369,7 @@ h2_rx_window_update(struct worker *wrk, struct h2_sess *h2, struct h2_req *r2) ...@@ -369,7 +369,7 @@ h2_rx_window_update(struct worker *wrk, struct h2_sess *h2, struct h2_req *r2)
Lck_Lock(&h2->sess->mtx); Lck_Lock(&h2->sess->mtx);
r2->t_window += wu; r2->t_window += wu;
if (r2 == h2->req0) if (r2 == h2->req0)
AZ(pthread_cond_broadcast(h2->cond)); AZ(pthread_cond_broadcast(h2->winupd_cond));
else if (r2->cond != NULL) else if (r2->cond != NULL)
AZ(pthread_cond_signal(r2->cond)); AZ(pthread_cond_signal(r2->cond));
Lck_Unlock(&h2->sess->mtx); Lck_Unlock(&h2->sess->mtx);
......
...@@ -213,7 +213,7 @@ h2_do_window(struct worker *wrk, struct h2_req *r2, ...@@ -213,7 +213,7 @@ h2_do_window(struct worker *wrk, struct h2_req *r2,
r2->cond = NULL; r2->cond = NULL;
} }
while (h2->req0->t_window <= 0 && h2_errcheck(r2, h2) == 0) { 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) { if (h2_errcheck(r2, h2) == 0) {
......
...@@ -123,6 +123,7 @@ h2_init_sess(const struct worker *wrk, struct sess *sp, ...@@ -123,6 +123,7 @@ h2_init_sess(const struct worker *wrk, struct sess *sp,
h2->htc->rfd = &sp->fd; h2->htc->rfd = &sp->fd;
h2->sess = sp; h2->sess = sp;
h2->rxthr = pthread_self(); h2->rxthr = pthread_self();
AZ(pthread_cond_init(h2->winupd_cond, NULL));
VTAILQ_INIT(&h2->streams); VTAILQ_INIT(&h2->streams);
VTAILQ_INIT(&h2->txqueue); VTAILQ_INIT(&h2->txqueue);
h2_local_settings(&h2->local_settings); h2_local_settings(&h2->local_settings);
...@@ -150,6 +151,7 @@ h2_del_sess(struct worker *wrk, struct h2_sess *h2, enum sess_close reason) ...@@ -150,6 +151,7 @@ h2_del_sess(struct worker *wrk, struct h2_sess *h2, enum sess_close reason)
assert(VTAILQ_EMPTY(&h2->streams)); assert(VTAILQ_EMPTY(&h2->streams));
VHT_Fini(h2->dectbl); VHT_Fini(h2->dectbl);
AZ(pthread_cond_destroy(h2->winupd_cond));
req = h2->srq; req = h2->srq;
AZ(req->ws->r); AZ(req->ws->r);
sp = h2->sess; sp = h2->sess;
...@@ -404,7 +406,7 @@ h2_new_session(struct worker *wrk, void *arg) ...@@ -404,7 +406,7 @@ h2_new_session(struct worker *wrk, void *arg)
if (r2->cond != NULL) if (r2->cond != NULL)
AZ(pthread_cond_signal(r2->cond)); AZ(pthread_cond_signal(r2->cond));
} }
AZ(pthread_cond_broadcast(h2->cond)); AZ(pthread_cond_broadcast(h2->winupd_cond));
Lck_Unlock(&h2->sess->mtx); Lck_Unlock(&h2->sess->mtx);
while (1) { while (1) {
again = 0; again = 0;
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment