[master] 55e4a206e Revert "vbe: Try fetching beresp when sending bereq failed"
Nils Goroll
nils.goroll at uplex.de
Tue May 31 10:06:04 UTC 2022
commit 55e4a206e85b8fa7f88b9d2b4dc04ef5e35194bb
Author: Nils Goroll <nils.goroll at uplex.de>
Date: Tue May 31 12:04:38 2022 +0200
Revert "vbe: Try fetching beresp when sending bereq failed"
Only momentarily until we understand and fix the newly introduced
issues.
This reverts commit f0ee94ecf548e936ce64c0cfb484877dfb0e4b88.
Ref #3813
Reopen #3761
diff --git a/bin/varnishd/cache/cache_backend.c b/bin/varnishd/cache/cache_backend.c
index 0e328bb18..7ac117cc4 100644
--- a/bin/varnishd/cache/cache_backend.c
+++ b/bin/varnishd/cache/cache_backend.c
@@ -243,8 +243,7 @@ vbe_dir_finish(VRT_CTX, VCL_BACKEND d)
AZ(pfd);
Lck_Lock(bp->director->mtx);
} else {
- assert(PFD_State(pfd) == PFD_STATE_USED);
- AZ(bo->send_failed);
+ assert (PFD_State(pfd) == PFD_STATE_USED);
VSLb(bo->vsl, SLT_BackendClose, "%d %s recycle", *PFD_Fd(pfd),
VRT_BACKEND_string(d));
Lck_Lock(bp->director->mtx);
@@ -269,7 +268,6 @@ vbe_dir_gethdrs(VRT_CTX, VCL_BACKEND d)
struct pfd *pfd;
struct busyobj *bo;
struct worker *wrk;
- stream_close_t sc;
CHECK_OBJ_NOTNULL(ctx, VRT_CTX_MAGIC);
CHECK_OBJ_NOTNULL(d, DIRECTOR_MAGIC);
@@ -277,7 +275,6 @@ vbe_dir_gethdrs(VRT_CTX, VCL_BACKEND d)
CHECK_OBJ_NOTNULL(bo, BUSYOBJ_MAGIC);
if (bo->htc != NULL)
CHECK_OBJ_NOTNULL(bo->htc->doclose, STREAM_CLOSE_MAGIC);
- AZ(bo->send_failed);
wrk = ctx->bo->wrk;
CHECK_OBJ_NOTNULL(wrk, WORKER_MAGIC);
CAST_OBJ_NOTNULL(bp, d->priv, BACKEND_MAGIC);
@@ -304,10 +301,8 @@ vbe_dir_gethdrs(VRT_CTX, VCL_BACKEND d)
i = V1F_SendReq(wrk, bo, &bo->acct.bereq_hdrbytes,
&bo->acct.bereq_bodybytes);
- if (PFD_State(pfd) != PFD_STATE_USED) {
- if (bo->send_failed)
- (void)VCP_Wait(wrk, pfd, VTIM_real());
- else if (VCP_Wait(wrk, pfd, VTIM_real() +
+ if (i == 0 && PFD_State(pfd) != PFD_STATE_USED) {
+ if (VCP_Wait(wrk, pfd, VTIM_real() +
bo->htc->first_byte_timeout) != 0) {
bo->htc->doclose = SC_RX_TIMEOUT;
VSLb(bo->vsl, SLT_FetchError,
@@ -316,25 +311,17 @@ vbe_dir_gethdrs(VRT_CTX, VCL_BACKEND d)
}
}
- if (bo->htc->doclose == SC_NULL)
+ if (bo->htc->doclose == SC_NULL) {
assert(PFD_State(pfd) == PFD_STATE_USED);
-
- sc = bo->htc->doclose;
- if (i == 0 || bo->send_failed) {
- i = V1F_FetchRespHdr(bo);
if (i == 0)
+ i = V1F_FetchRespHdr(bo);
+ if (i == 0) {
AN(bo->htc->priv);
+ return (0);
+ }
}
CHECK_OBJ_NOTNULL(bo->htc->doclose, STREAM_CLOSE_MAGIC);
- if (bo->send_failed) {
- assert(sc != SC_NULL);
- bo->htc->doclose = sc;
- }
-
- if (i == 0)
- return (0);
-
/*
* If we recycled a backend connection, there is a finite chance
* that the backend closed it before we got the bereq to it.
diff --git a/bin/varnishd/cache/cache_fetch.c b/bin/varnishd/cache/cache_fetch.c
index 047dec969..fde7e29a9 100644
--- a/bin/varnishd/cache/cache_fetch.c
+++ b/bin/varnishd/cache/cache_fetch.c
@@ -330,7 +330,6 @@ vbf_stp_retry(struct worker *wrk, struct busyobj *bo)
bo->do_esi = 0;
bo->do_stream = 1;
bo->was_304 = 0;
- bo->send_failed = 0;
bo->err_code = 0;
bo->err_reason = NULL;
if (bo->htc != NULL)
@@ -548,8 +547,6 @@ vbf_stp_startfetch(struct worker *wrk, struct busyobj *bo)
}
if (bo->uncacheable)
oc->flags |= OC_F_HFM;
- if (bo->send_failed)
- HSH_Kill(oc);
assert(wrk->handling == VCL_RET_DELIVER);
@@ -749,7 +746,7 @@ vbf_stp_fetchend(struct worker *wrk, struct busyobj *bo)
ObjSetState(wrk, oc, BOS_FINISHED);
VSLb_ts_busyobj(bo, "BerespBody", W_TIM_real(wrk));
- if (bo->stale_oc != NULL && !bo->send_failed)
+ if (bo->stale_oc != NULL)
HSH_Kill(bo->stale_oc);
return (F_STP_DONE);
}
diff --git a/bin/varnishd/http1/cache_http1_fetch.c b/bin/varnishd/http1/cache_http1_fetch.c
index 8a3245f4e..7d0e6c015 100644
--- a/bin/varnishd/http1/cache_http1_fetch.c
+++ b/bin/varnishd/http1/cache_http1_fetch.c
@@ -119,6 +119,15 @@ V1F_SendReq(struct worker *wrk, struct busyobj *bo, uint64_t *ctr_hdrbytes,
bo->no_retry = "req.body not cached";
if (bo->req->req_body_status == BS_ERROR) {
+ /*
+ * XXX: (#2332) We should test to see if the backend
+ * XXX: sent us some headers explaining why.
+ * XXX: This is hard because of the mistaken API split
+ * XXX: between cache_backend.c and V1F, and therefore
+ * XXX: Parked in this comment, pending renovation of
+ * XXX: the VDI/backend-protocol API to allow non-H1
+ * XXX: backends.
+ */
assert(i < 0);
VSLb(bo->vsl, SLT_FetchError,
"req.body read error: %d (%s)",
@@ -149,15 +158,10 @@ V1F_SendReq(struct worker *wrk, struct busyobj *bo, uint64_t *ctr_hdrbytes,
errno, VAS_errtxt(errno), sc->desc);
VSLb_ts_busyobj(bo, "Bereq", W_TIM_real(wrk));
htc->doclose = sc;
- /* NB: only raise the flag if we managed to at least send
- * the request headers.
- */
- bo->send_failed = bytes >= hdrbytes;
return (-1);
}
CHECK_OBJ_NOTNULL(sc, STREAM_CLOSE_MAGIC);
VSLb_ts_busyobj(bo, "Bereq", W_TIM_real(wrk));
- bo->send_failed = 0;
return (0);
}
@@ -167,7 +171,7 @@ V1F_FetchRespHdr(struct busyobj *bo)
struct http *hp;
int i;
- vtim_real t;
+ double t;
struct http_conn *htc;
enum htc_status_e hs;
@@ -186,9 +190,7 @@ V1F_FetchRespHdr(struct busyobj *bo)
CHECK_OBJ_NOTNULL(htc, HTTP_CONN_MAGIC);
CHECK_OBJ_NOTNULL(bo->htc, HTTP_CONN_MAGIC);
- t = VTIM_real();
- if (!bo->send_failed)
- t += htc->first_byte_timeout;
+ t = VTIM_real() + htc->first_byte_timeout;
hs = HTC_RxStuff(htc, HTTP1_Complete, NULL, NULL,
t, NAN, htc->between_bytes_timeout, cache_param->http_resp_size);
if (hs != HTC_S_COMPLETE) {
@@ -212,8 +214,6 @@ V1F_FetchRespHdr(struct busyobj *bo)
htc->doclose = SC_RX_OVERFLOW;
break;
case HTC_S_IDLE:
- if (bo->send_failed)
- break;
VSLb(bo->vsl, SLT_FetchError, "first byte timeout");
htc->doclose = SC_RX_TIMEOUT;
break;
diff --git a/bin/varnishtest/tests/b00073.vtc b/bin/varnishtest/tests/b00073.vtc
index 3aea0207e..3372ba094 100644
--- a/bin/varnishtest/tests/b00073.vtc
+++ b/bin/varnishtest/tests/b00073.vtc
@@ -27,20 +27,13 @@ server s1 {
expect req.http.unset-connection == true
txresp
expect_close
-
- accept
- rxreq
} -start
varnish v1 -vcl+backend {
- backend bad { .host = "${bad_backend}"; }
sub vcl_recv {
return (pass);
}
sub vcl_backend_fetch {
- if (bereq.http.fail == "send") {
- set bereq.backend = bad;
- }
set bereq.http.connection = bereq.http.bereq-connection;
}
sub vcl_backend_response {
@@ -68,19 +61,9 @@ client c1 {
txreq -hdr "bereq-connection: close" -hdr "unset-connection: true"
rxresp
expect resp.status == 200
-
- txreq -hdr "fail: fetch"
- rxresp
- expect resp.status == 503
} -run
server s1 -wait
-client c2 {
- txreq -hdr "fail: send"
- rxresp
- expect resp.status == 503
-} -run
-
varnish v1 -expect MAIN.backend_recycle == 0
varnish v1 -expect VBE.vcl1.s1.conn == 0
diff --git a/bin/varnishtest/tests/f00001.vtc b/bin/varnishtest/tests/f00001.vtc
index 9d52b06fc..2c756c9cd 100644
--- a/bin/varnishtest/tests/f00001.vtc
+++ b/bin/varnishtest/tests/f00001.vtc
@@ -4,7 +4,7 @@ varnishtest "Check that we handle bogusly large chunks correctly"
server s1 {
rxreq
- txresp -status 400
+ txresp
} -start
varnish v1 -vcl+backend {
@@ -18,7 +18,7 @@ client c1 {
send "0\r\n\r\n"
rxresp
- expect resp.status == 400
+ expect resp.status == 503
} -run
# Check that the published workaround does not cause harm
diff --git a/bin/varnishtest/tests/r02332.vtc b/bin/varnishtest/tests/r02332.vtc
deleted file mode 100644
index b76d8a3d2..000000000
--- a/bin/varnishtest/tests/r02332.vtc
+++ /dev/null
@@ -1,99 +0,0 @@
-varnishtest "Fetch beresp despite failure to send bereq"
-
-barrier b1 cond 2
-barrier b2 cond 2
-
-# bo->send_failed
-
-server s1 {
- rxreqhdrs
- txresp -status 400 -body "Invalid request"
-} -start
-
-varnish v1 -vcl+backend {
- sub vcl_backend_fetch {
- if (bereq.http.ignore == "bgfetch") {
- return (abandon);
- }
- return (fetch); # don't unset bereq.body
- }
-} -start
-
-logexpect l1 -v v1 -g raw -i BackendClose {
- expect 0 1002 BackendClose "s1 close"
- expect 1 1007 BackendClose "s1 close"
-} -start
-
-client c1 {
- txreq -method POST -hdr "Transfer-Encoding: chunked"
- chunkedlen 100
- barrier b1 sync
- non_fatal
- chunkedlen 100
- chunkedlen 0
- fatal
- rxresp
- expect resp.status == 400
- expect resp.body == "Invalid request"
-} -start
-
-server s1 -wait
-barrier b1 sync
-client c1 -wait
-
-# bo->send_failed && 304
-
-server s1 {
- rxreq
- txresp -hdr "Cache-Control: public, max-age=1" -hdr {Etag: "abc"} \
- -hdr "version: original" -body can-revalidate
-
- rxreqhdrs
- expect req.http.if-none-match == {"abc"}
- txresp -status 304 -hdr "Cache-Control: public" -hdr {Etag: "abc"} \
- -hdr "version: refreshed"
-} -start
-
-client c2 {
- txreq -hdr "Transfer-Encoding: chunked"
- chunkedlen 100
- chunkedlen 100
- chunkedlen 0
- rxresp
- expect resp.http.etag == {"abc"}
- expect resp.http.version == original
- expect resp.body == can-revalidate
-
- delay 2
-
- # bereq.send_failed during grace
- txreq -hdr "Transfer-Encoding: chunked"
- chunkedlen 100
- barrier b2 sync
- non_fatal
- chunkedlen 100
- chunkedlen 0
- fatal
- rxresp
- expect resp.status == 200
- expect resp.http.etag == {"abc"}
- expect resp.http.version == original
- expect resp.body == can-revalidate
-} -start
-
-server s1 -wait
-barrier b2 sync
-client c2 -wait
-
-client c3 {
- txreq -hdr "Transfer-Encoding: chunked" -hdr "ignore: bgfetch"
- chunkedlen 100
- chunkedlen 100
- chunkedlen 0
- rxresp
- expect resp.http.etag == {"abc"}
- expect resp.http.version == original
- expect resp.body == can-revalidate
-} -run
-
-logexpect l1 -wait
diff --git a/doc/sphinx/reference/directors.rst b/doc/sphinx/reference/directors.rst
index 30888a99e..66c0f406c 100644
--- a/doc/sphinx/reference/directors.rst
+++ b/doc/sphinx/reference/directors.rst
@@ -195,9 +195,7 @@ statistics, it is essentially a director which state is a ``struct
backend``. Varnish native backends currently speak HTTP/1 over TCP or
UDS, and as such, you need to make your own custom backend if you want
Varnish to do otherwise such as connect over UDP or speak a different
-protocol. A custom backend implementation must implement the ``gethdrs``
-method, and optionally ``http1pipe``. It is the responsibility of the
-custom backend to raise the ``send_failed`` flag from ``struct busyobj``.
+protocol.
If you want to leverage probes declarations in VCL, which have the advantage of
being reusable since they are only specifications, you can. However, you need
diff --git a/include/tbl/bo_flags.h b/include/tbl/bo_flags.h
index 624494d43..4a4f1dd4b 100644
--- a/include/tbl/bo_flags.h
+++ b/include/tbl/bo_flags.h
@@ -44,7 +44,6 @@ BO_FLAG(was_304, 0, 1, 0, 0, "")
BO_FLAG(is_bgfetch, 1, 0, 0, 0, "")
BO_FLAG(is_hitmiss, 1, 0, 0, 0, "")
BO_FLAG(is_hitpass, 1, 0, 0, 0, "")
-BO_FLAG(send_failed, 0, 0, 0, 0, "")
#undef BO_FLAG
/*lint -restore */
More information about the varnish-commit
mailing list