[master] c72674662 fix between bytes timeout vs total timeout
Nils Goroll
nils.goroll at uplex.de
Thu Feb 21 17:09:06 UTC 2019
commit c726746621f7b87a069fdc0ca83bef82b4cefd5b
Author: Nils Goroll <nils.goroll at uplex.de>
Date: Thu Feb 21 17:59:51 2019 +0100
fix between bytes timeout vs total timeout
On the client side, we impose a total timeout, yet on the client side we
use between_bytes_timeout and do not care about the total (<- we might
want to reconsider this).
Yet HTC_RxStuff only implemented a total timeout, for which we
effectively used first_byte_timeout + between_bytes_timeout. Yet if
first_byte_timeout was not used up, the effective timeout between bytes
could be substantially longer than between_bytes_timeout (initially
analyzed by @daghf).
We now add a duration argument td to HTC_RxStuff which will be used in
addition to (or instead of) the existing total timeout tn. Either td or
tn must be given.
Testcase originally by @fgsch, slightly modified to avoid an assertion
failure in vtc_server due to the connection being closed by varnish.
Fixes #2395
diff --git a/bin/varnishd/cache/cache_session.c b/bin/varnishd/cache/cache_session.c
index bf3fd14ff..259110d7b 100644
--- a/bin/varnishd/cache/cache_session.c
+++ b/bin/varnishd/cache/cache_session.c
@@ -247,12 +247,14 @@ HTC_RxPipeline(struct http_conn *htc, void *p)
* *t1 becomes time of first non-idle rx
* *t2 becomes time of complete rx
* ti is when we return IDLE if nothing has arrived
- * tn is when we timeout on non-complete
+ * tn is when we timeout on non-complete (total timeout)
+ * td is max timeout between reads
*/
enum htc_status_e
HTC_RxStuff(struct http_conn *htc, htc_complete_f *func,
- vtim_real *t1, vtim_real *t2, vtim_real ti, vtim_real tn, int maxbytes)
+ vtim_real *t1, vtim_real *t2, vtim_real ti, vtim_real tn, vtim_dur td,
+ int maxbytes)
{
vtim_dur tmo;
vtim_real now;
@@ -267,7 +269,7 @@ HTC_RxStuff(struct http_conn *htc, htc_complete_f *func,
assert(htc->rxbuf_b <= htc->rxbuf_e);
assert(htc->rxbuf_e <= htc->ws->r);
- AZ(isnan(tn));
+ AZ(isnan(tn) && isnan(td));
if (t1 != NULL)
assert(isnan(*t1));
@@ -309,9 +311,18 @@ HTC_RxStuff(struct http_conn *htc, htc_complete_f *func,
else
WRONG("htc_status_e");
- tmo = tn - now;
- if (!isnan(ti) && ti < tn && hs == HTC_S_EMPTY)
+ if (hs == HTC_S_EMPTY && !isnan(ti) && (isnan(tn) || ti < tn))
tmo = ti - now;
+ else if (isnan(tn))
+ tmo = td;
+ else if (isnan(td))
+ tmo = tn - now;
+ else if (td < tn - now)
+ tmo = td;
+ else
+ tmo = tn - now;
+
+ AZ(isnan(tmo));
z = maxbytes - (htc->rxbuf_e - htc->rxbuf_b);
if (z <= 0) {
/* maxbytes reached but not HTC_S_COMPLETE. Return
@@ -328,14 +339,11 @@ HTC_RxStuff(struct http_conn *htc, htc_complete_f *func,
} else if (z > 0)
htc->rxbuf_e += z;
else if (z == -2) {
- if (hs == HTC_S_EMPTY && ti <= now) {
- WS_ReleaseP(htc->ws, htc->rxbuf_b);
+ WS_ReleaseP(htc->ws, htc->rxbuf_b);
+ if (hs == HTC_S_EMPTY)
return (HTC_S_IDLE);
- }
- if (tn <= now) {
- WS_ReleaseP(htc->ws, htc->rxbuf_b);
+ else
return (HTC_S_TIMEOUT);
- }
}
}
}
diff --git a/bin/varnishd/cache/cache_varnishd.h b/bin/varnishd/cache/cache_varnishd.h
index 2fdd427ba..2aab94ab7 100644
--- a/bin/varnishd/cache/cache_varnishd.h
+++ b/bin/varnishd/cache/cache_varnishd.h
@@ -342,7 +342,8 @@ const char * HTC_Status(enum htc_status_e);
void HTC_RxInit(struct http_conn *htc, struct ws *ws);
void HTC_RxPipeline(struct http_conn *htc, void *);
enum htc_status_e HTC_RxStuff(struct http_conn *, htc_complete_f *,
- vtim_real *t1, vtim_real *t2, vtim_real ti, vtim_real tn, int maxbytes);
+ vtim_real *t1, vtim_real *t2, vtim_real ti, vtim_real tn, vtim_dur td,
+ int maxbytes);
#define SESS_ATTR(UP, low, typ, len) \
int SES_Set_##low(const struct sess *sp, const typ *src); \
diff --git a/bin/varnishd/http1/cache_http1_fetch.c b/bin/varnishd/http1/cache_http1_fetch.c
index fa456a408..f4ee4ebf0 100644
--- a/bin/varnishd/http1/cache_http1_fetch.c
+++ b/bin/varnishd/http1/cache_http1_fetch.c
@@ -178,7 +178,7 @@ V1F_FetchRespHdr(struct busyobj *bo)
t = VTIM_real() + htc->first_byte_timeout;
hs = HTC_RxStuff(htc, HTTP1_Complete, NULL, NULL,
- t, t + htc->between_bytes_timeout, cache_param->http_resp_size);
+ t, NAN, htc->between_bytes_timeout, cache_param->http_resp_size);
if (hs != HTC_S_COMPLETE) {
bo->acct.beresp_hdrbytes +=
htc->rxbuf_e - htc->rxbuf_b;
diff --git a/bin/varnishd/http1/cache_http1_fsm.c b/bin/varnishd/http1/cache_http1_fsm.c
index 14b36d4d2..c2386dcb1 100644
--- a/bin/varnishd/http1/cache_http1_fsm.c
+++ b/bin/varnishd/http1/cache_http1_fsm.c
@@ -332,6 +332,7 @@ HTTP1_Session(struct worker *wrk, struct req *req)
&req->t_first, &req->t_req,
sp->t_idle + cache_param->timeout_linger,
sp->t_idle + cache_param->timeout_idle,
+ NAN,
cache_param->http_req_size);
AZ(req->htc->ws->r);
if (hs < HTC_S_EMPTY) {
diff --git a/bin/varnishd/http2/cache_http2_proto.c b/bin/varnishd/http2/cache_http2_proto.c
index bb2de4cf4..e824eadb8 100644
--- a/bin/varnishd/http2/cache_http2_proto.c
+++ b/bin/varnishd/http2/cache_http2_proto.c
@@ -1080,7 +1080,7 @@ h2_rxframe(struct worker *wrk, struct h2_sess *h2)
h2->sess->t_idle = VTIM_real();
hs = HTC_RxStuff(h2->htc, h2_frame_complete,
NULL, NULL, NAN,
- h2->sess->t_idle + cache_param->timeout_idle,
+ h2->sess->t_idle + cache_param->timeout_idle, NAN,
h2->local_settings.max_frame_size + 9);
switch (hs) {
case HTC_S_COMPLETE:
diff --git a/bin/varnishd/http2/cache_http2_session.c b/bin/varnishd/http2/cache_http2_session.c
index eee053931..ed73f5f31 100644
--- a/bin/varnishd/http2/cache_http2_session.c
+++ b/bin/varnishd/http2/cache_http2_session.c
@@ -291,7 +291,7 @@ h2_ou_session(struct worker *wrk, struct h2_sess *h2,
/* Wait for PRISM response */
hs = HTC_RxStuff(h2->htc, H2_prism_complete,
- NULL, NULL, NAN, h2->sess->t_idle + cache_param->timeout_idle,
+ NULL, NULL, NAN, h2->sess->t_idle + cache_param->timeout_idle, NAN,
sizeof H2_prism);
if (hs != HTC_S_COMPLETE) {
VSLb(h2->vsl, SLT_Debug, "H2: No/Bad OU PRISM (hs=%d)", hs);
diff --git a/bin/varnishd/proxy/cache_proxy_proto.c b/bin/varnishd/proxy/cache_proxy_proto.c
index fc4c451b3..86fc2f97d 100644
--- a/bin/varnishd/proxy/cache_proxy_proto.c
+++ b/bin/varnishd/proxy/cache_proxy_proto.c
@@ -554,7 +554,7 @@ vpx_new_session(struct worker *wrk, void *arg)
HTC_RxInit(req->htc, req->ws);
hs = HTC_RxStuff(req->htc, vpx_complete,
- NULL, NULL, NAN, sp->t_idle + cache_param->timeout_idle,
+ NULL, NULL, NAN, sp->t_idle + cache_param->timeout_idle, NAN,
1024); // XXX ?
if (hs != HTC_S_COMPLETE) {
Req_Release(req);
diff --git a/bin/varnishtest/tests/r02395.vtc b/bin/varnishtest/tests/r02395.vtc
new file mode 100644
index 000000000..99461e16c
--- /dev/null
+++ b/bin/varnishtest/tests/r02395.vtc
@@ -0,0 +1,22 @@
+varnishtest "Test between_bytes_timeout works fetching headers"
+
+server s1 {
+ rxreq
+ send "HTTP/1.0 "
+ delay 2
+} -start
+
+varnish v1 -vcl+backend {
+ sub vcl_recv { return (pass); }
+ sub vcl_backend_fetch { set bereq.between_bytes_timeout = 1s; }
+} -start
+
+varnish v1 -cliok "param.set debug +syncvsl"
+
+client c1 {
+ txreq
+ rxresp
+ expect resp.status == 503
+} -run
+
+server s1 -wait
More information about the varnish-commit
mailing list