BUG/MINOR: http-ana: count response body failures only once in http_fail_cnt

In http_response_forward_body(), the return_srv_abort and return_bad_res
labels incremented the http_fail_cnt counters of the tracked stick counters,
then continued to the return_error label, which incremented them again. So a
server aborting in the middle of the body, or an invalid response body, was
counted twice.And since return_cli_abort and return_int_err also end at
return_error, a client aborting a download, or an internal error, was counted
as a server failure, while the counter is documented to only count the
failures caused by the servers.

Let's remove the increment from return_error. A reg-test is added with a
server closing in the middle of the body and a client aborting a download.

This was introduced by f2b02cfd9 ("MAJOR: http-ana: Review error handling
during HTTP payload forwarding") so this must be backported up to 2.8.

diff --git a/reg-tests/http-messaging/http_fail_cnt_body.vtc b/reg-tests/http-messaging/http_fail_cnt_body.vtc
new file mode 100644 (file)
index 0000000..aa25012
--- /dev/null
+++ b/reg-tests/http-messaging/http_fail_cnt_body.vtc
@@ -0,0 +1,84 @@
+varnishtest "http_fail_cnt on errors while the response body is forwarded"
+
+# The http_fail_cnt counter counts the responses which failed because of the
+# server. An abort of the server in the middle of the body is one failure
+# and must be counted once, while a client aborting the download is not a
+# server failure at all.
+#
+# The cases are:
+#   - /srv: the server announces 100 bytes, sends 3, waits for the client
+#     to have read them and closes: http_fail_cnt must be 1;
+#   - /cli: the server announces 100000 bytes and sends 100, the client
+#     reads them and closes: http_fail_cnt must be 0.
+
+feature ignore_unknown_macro
+
+barrier b1 cond 2 -cyclic
+
+server s1 {
+    rxreq
+    expect req.url == "/srv"
+    txresp -nolen -hdr "content-length: 100" -body "abc"
+    barrier b1 sync
+} -start
+
+server s2 {
+    rxreq
+    expect req.url == "/cli"
+    txresp -nolen -hdr "content-length: 100000" -bodylen 100
+    barrier b1 sync
+    expect_close
+} -start
+
+haproxy h1 -conf {
+    defaults
+        mode http
+        timeout connect "${HAPROXY_TEST_TIMEOUT-5s}"
+        timeout client  "${HAPROXY_TEST_TIMEOUT-5s}"
+        timeout server  "${HAPROXY_TEST_TIMEOUT-5s}"
+
+    backend st
+        stick-table type string size 10 store http_fail_cnt
+
+    frontend fe
+        bind "fd@${fe}"
+        http-request track-sc0 path table st
+        use_backend be2 if { path /cli }
+        default_backend be1
+
+    backend be1
+        server s1 ${s1_addr}:${s1_port}
+
+    backend be2
+        server s2 ${s2_addr}:${s2_port}
+} -start
+
+client c1 -connect ${h1_fe_sock} {
+    txreq -url "/srv"
+    rxresphdrs
+    expect resp.status == 200
+    recv 3
+    barrier b1 sync
+    expect_close
+} -run
+
+client c2 -connect ${h1_fe_sock} {
+    txreq -url "/cli"
+    rxresphdrs
+    expect resp.status == 200
+    recv 100
+    barrier b1 sync
+} -run
+
+# let the second stream see the client close
+delay 0.5
+
+haproxy h1 -cli {
+    send "show table st"
+    expect ~ "key=/srv .*http_fail_cnt=1\n"
+}
+
+haproxy h1 -cli {
+    send "show table st"
+    expect ~ "key=/cli .*http_fail_cnt=0\n"
+}

diff --git a/src/http_ana.c b/src/http_ana.c
index cc5e0e58a77cce51ed9a63b76b0b894b9f4d99bb..a23cb20f1aa367b405de57af61c7c49fc865480f 100644 (file)
--- a/src/http_ana.c
+++ b/src/http_ana.c
@@ -2377,7 +2377,6 @@ int http_response_forward_body(struct stream *s, struct channel *res, int an_bit
        /* don't send any error message as we're in the body */
        http_set_term_flags(s);
        http_reply_and_close(s, txn->status, NULL);
-       stream_inc_http_fail_ctr(s);
        DBG_TRACE_DEVEL("leaving on error",
                        STRM_EV_STRM_ANA|STRM_EV_HTTP_ANA|STRM_EV_HTTP_ERR, s, txn);
        return 0;