Skip to content

Commit

Permalink
respect send_timeout for "dripping" http1 writes
Browse files Browse the repository at this point in the history
Previously, we only checked `v1l->deadline` (which gets initialized
from the `send_timeout` session attribute or parameter) only for short
writes, so for successful "dripping" http1 writes (streaming from a
backend busy object with small chunks), we did not respect the
timeout.

This patch restructures `V1L_Flush()` to always check the deadline
before a write by turning a `while() { ... }` into a `do { ... }
while` with the same continuation criteria: `while (i != v1l->liov)`
is turned into `if (i == v1l->liov) break;` and `while (i > 0 || errno
== EWOULDBLOCK)` is kept to retry short writes.

This also reduces the `writev()` call sites to one.

Fixes varnishcache#3189
  • Loading branch information
nigoroll committed Mar 3, 2020
1 parent 9201fdb commit 7ff216e
Show file tree
Hide file tree
Showing 3 changed files with 48 additions and 20 deletions.
34 changes: 18 additions & 16 deletions bin/varnishd/http1/cache_http1_line.c
Expand Up @@ -200,18 +200,8 @@ V1L_Flush(const struct worker *wrk)
v1l->iov[v1l->ciov].iov_len = 0;
}

i = writev(*v1l->wfd, v1l->iov, v1l->niov);
if (i > 0)
v1l->cnt += i;
while (i != v1l->liov && (i > 0 || errno == EWOULDBLOCK)) {
/* Remove sent data from start of I/O vector,
* then retry; we hit a timeout, and some data
* may have been sent.
*
* XXX: Add a "minimum sent data per timeout
* counter to prevent slowloris attacks
*/

i = 0;
do {
if (VTIM_real() > v1l->deadline) {
VSLb(v1l->vsl, SLT_Debug,
"Hit total send timeout, "
Expand All @@ -221,16 +211,28 @@ V1L_Flush(const struct worker *wrk)
break;
}

i = writev(*v1l->wfd, v1l->iov, v1l->niov);
if (i > 0)
v1l->cnt += i;

if (i == v1l->liov)
break;

/* we hit a timeout, and some data may have been sent:
* Remove sent data from start of I/O vector, then retry
*
* XXX: Add a "minimum sent data per timeout counter to
* prevent slowloris attacks
*/

VSLb(v1l->vsl, SLT_Debug,
"Hit idle send timeout, wrote = %zd/%zd; retrying",
i, v1l->liov);

if (i > 0)
v1l_prune(v1l, i);
i = writev(*v1l->wfd, v1l->iov, v1l->niov);
if (i > 0)
v1l->cnt += i;
}
} while (i > 0 || errno == EWOULDBLOCK);

if (i <= 0) {
VSLb(v1l->vsl, SLT_Debug,
"Write error, retval = %zd, len = %zd, errno = %s",
Expand Down
28 changes: 28 additions & 0 deletions bin/varnishtest/tests/r03189.vtc
@@ -0,0 +1,28 @@
varnishtest "h1 send_timeout and streaming of dripping chunks"

barrier b cond 2

server s1 {
rxreq
txresp -nolen -hdr "Transfer-Encoding: chunked"
chunkedlen 1
delay 1
non_fatal
barrier b sync
chunkedlen 1
delay 1
chunkedlen 0
} -start

varnish v1 \
-arg "-p idle_send_timeout=.1" \
-arg "-p send_timeout=.8" \
-vcl+backend { } -start

client c1 {
txreq
rxresphdrs
rxchunk
barrier b sync
expect_close
} -run
6 changes: 2 additions & 4 deletions bin/varnishtest/tests/s00011.vtc
@@ -1,10 +1,8 @@
varnishtest "backend send timeouts"

server s1 {
non_fatal
rxreq
expect req.method == POST
expect req.body == helloworld
txresp
} -start

varnish v1 -vcl+backend "" -start
Expand All @@ -21,5 +19,5 @@ client c1 {
delay 2
send world
rxresp
expect resp.status == 200
expect resp.status == 503
} -run

0 comments on commit 7ff216e

Please sign in to comment.