From 4250083b875d3445ab6b3a5dca6e9228cef22d96 Mon Sep 17 00:00:00 2001 From: Dridi Boukelmoune Date: Fri, 1 Dec 2023 16:25:00 +0100 Subject: [PATCH] req_fsm: Use status 408 for reset streams The 503 synth and 500 minimal response status codes are too misleading in this context, where the failure is attributed to the client. Among existing 4XX status codes, this is the closest if we stretch the timeout definition to "didn't complete rapidly enough before the client went away". --- bin/varnishd/cache/cache_req_fsm.c | 13 ++++++++++--- bin/varnishtest/tests/t02025.vtc | 4 ++-- doc/sphinx/reference/vsl.rst | 3 ++- 3 files changed, 14 insertions(+), 6 deletions(-) diff --git a/bin/varnishd/cache/cache_req_fsm.c b/bin/varnishd/cache/cache_req_fsm.c index aa0a0db98fe..b49f4dadd2e 100644 --- a/bin/varnishd/cache/cache_req_fsm.c +++ b/bin/varnishd/cache/cache_req_fsm.c @@ -288,8 +288,13 @@ cnt_vclfail(struct worker *wrk, struct req *req) Req_Rollback(ctx); - req->err_code = 503; - req->err_reason = "VCL failed"; + if (req->req_reset) { + req->err_code = 408; + req->err_reason = "Client disconnected"; + } else { + req->err_code = 503; + req->err_reason = "VCL failed"; + } req->req_step = R_STP_SYNTH; req->doclose = SC_VCL_FAILURE; req->filter_list = NULL; @@ -305,6 +310,7 @@ cnt_synth(struct worker *wrk, struct req *req) { struct vsb *synth_body; ssize_t sz, szl; + uint16_t status; uint8_t *ptr; const char *body; @@ -339,7 +345,8 @@ cnt_synth(struct worker *wrk, struct req *req) } VSB_destroy(&synth_body); (void)VRB_Ignore(req); - (void)req->transport->minimal_response(req, 500); + status = req->req_reset ? 408 : 500; + (void)req->transport->minimal_response(req, status); req->doclose = SC_VCL_FAILURE; // XXX: Not necessary any more ? VSLb_ts_req(req, "Resp", W_TIM_real(wrk)); http_Teardown(req->resp); diff --git a/bin/varnishtest/tests/t02025.vtc b/bin/varnishtest/tests/t02025.vtc index 578dbf5c84b..4b83fe95b8b 100644 --- a/bin/varnishtest/tests/t02025.vtc +++ b/bin/varnishtest/tests/t02025.vtc @@ -46,7 +46,7 @@ varnish v1 -expect req_reset == 1 # is interpreted as before a second elapsed. Session VXIDs showing up # numerous times become increasingly more suspicious. The format can of # course be extended to add anything else useful for data mining. -shell -expect "1000 ${localhost}" { +shell -expect "1000 ${localhost} 408" { varnishncsa -n ${v1_name} -d \ - -q 'Timestamp:Reset[2] < 1.0' -F '%{VSL:Begin[2]}x %h' + -q 'Timestamp:Reset[2] < 1.0' -F '%{VSL:Begin[2]}x %h %s' } diff --git a/doc/sphinx/reference/vsl.rst b/doc/sphinx/reference/vsl.rst index 98c126d2d1d..e3c6d0db825 100644 --- a/doc/sphinx/reference/vsl.rst +++ b/doc/sphinx/reference/vsl.rst @@ -79,7 +79,8 @@ Restart Reset The client closed its connection, reset its stream or caused a stream error that forced Varnish to reset the stream. Request - processing is interrupted and considered failed. + processing is interrupted and considered failed, with a 408 + "Request Timeout" status code. Pipe handling timestamps ~~~~~~~~~~~~~~~~~~~~~~~~