MAJOR: http-ana: Review error handling during HTTP payload forwarding
The error handling in the HTTP payload forwarding is far to be ideal because
both sides (request and response) are tested each time. It is espcially ugly
on the request side. To report a server error instead of a client error,
there are some workarounds to delay the error handling. The reason is that
the request analyzer is evaluated before the response one. In addition,
errors are tested before the data analysis. It means it is possible to
truncate data because errors may be handled to early.
So the error handling at this stages was totally reviewed. Aborts are now
handled after the data analysis. We also stop to finish the response on
request error or the opposite. As a side effect, the HTTP_MSG_ERROR state is
now useless. As another side effect, the termination flags are now set by
the HTTP analysers and not process_stream().
diff --git a/reg-tests/http-messaging/http_request_buffer.vtc b/reg-tests/http-messaging/http_request_buffer.vtc
index 15ec540..302db4a 100644
--- a/reg-tests/http-messaging/http_request_buffer.vtc
+++ b/reg-tests/http-messaging/http_request_buffer.vtc
@@ -36,7 +36,7 @@
barrier b1 sync
recv
- expect ~ "[^:\\[ ]*\\[[0-9]*\\]: .* .* fe2 be1/<NOSRV> [0-9]*/-1/-1/-1/[0-9]* -1 .* - - CR-- .* .* \"POST /2 HTTP/1\\.1\""
+ expect ~ "[^:\\[ ]*\\[[0-9]*\\]: .* .* fe2 be1/<NOSRV> [0-9]*/-1/-1/-1/[0-9]* 400 .* - - CR-- .* .* \"POST /2 HTTP/1\\.1\""
} -start
haproxy h1 -conf {
diff --git a/src/http_ana.c b/src/http_ana.c
index 12dd487..6949562 100644
--- a/src/http_ana.c
+++ b/src/http_ana.c
@@ -984,35 +984,6 @@
if (htx->flags & HTX_FL_PROCESSING_ERROR)
goto return_int_err;
- if ((req->flags & (CF_READ_ERROR|CF_READ_TIMEOUT|CF_WRITE_ERROR|CF_WRITE_TIMEOUT)) ||
- ((req->flags & CF_SHUTW) && (req->to_forward || co_data(req)))) {
- /* Output closed while we were sending data. We must abort and
- * wake the other side up.
- *
- * If we have finished to send the request and the response is
- * still in progress, don't catch write error on the request
- * side if it is in fact a read error on the server side.
- */
- if (msg->msg_state == HTTP_MSG_DONE && (s->res.flags & CF_READ_ERROR) && s->res.analysers)
- return 0;
-
- /* Don't abort yet if we had L7 retries activated and it
- * was a write error, we may recover.
- */
- if (!(req->flags & (CF_READ_ERROR | CF_READ_TIMEOUT)) &&
- (txn->flags & TX_L7_RETRY)) {
- DBG_TRACE_DEVEL("leaving on L7 retry",
- STRM_EV_STRM_ANA|STRM_EV_HTTP_ANA|STRM_EV_HTTP_ERR, s, txn);
- return 0;
- }
- msg->msg_state = HTTP_MSG_ERROR;
- http_end_request(s);
- http_end_response(s);
- DBG_TRACE_DEVEL("leaving on error",
- STRM_EV_STRM_ANA|STRM_EV_HTTP_ANA|STRM_EV_HTTP_ERR, s, txn);
- return 1;
- }
-
/* Note that we don't have to send 100-continue back because we don't
* need the data to complete our job, and it's up to the server to
* decide whether to return 100, 417 or anything else in return of
@@ -1100,17 +1071,14 @@
if (!(txn->flags & TX_CON_WANT_TUN))
channel_dont_close(req);
+ if ((req->flags & CF_SHUTW) && co_data(req)) {
+ /* request errors are most likely due to the server aborting the
+ * transfer. */
+ goto return_srv_abort;
+ }
+
http_end_request(s);
if (!(req->analysers & an_bit)) {
- http_end_response(s);
- if (unlikely(msg->msg_state == HTTP_MSG_ERROR)) {
- if (req->flags & CF_SHUTW) {
- /* request errors are most likely due to the
- * server aborting the transfer. */
- goto return_srv_abort;
- }
- goto return_bad_req;
- }
DBG_TRACE_LEAVE(STRM_EV_STRM_ANA|STRM_EV_HTTP_ANA, s, txn);
return 1;
}
@@ -1180,7 +1148,7 @@
if (objt_server(s->target))
_HA_ATOMIC_INC(&__objt_server(s->target)->counters.cli_aborts);
if (!(s->flags & SF_ERR_MASK))
- s->flags |= SF_ERR_CLICL;
+ s->flags |= ((req->flags & CF_READ_TIMEOUT) ? SF_ERR_CLITO : SF_ERR_CLICL);
status = 400;
goto return_prx_cond;
@@ -1192,7 +1160,7 @@
if (objt_server(s->target))
_HA_ATOMIC_INC(&__objt_server(s->target)->counters.srv_aborts);
if (!(s->flags & SF_ERR_MASK))
- s->flags |= SF_ERR_SRVCL;
+ s->flags |= ((req->flags & CF_WRITE_TIMEOUT) ? SF_ERR_SRVTO : SF_ERR_SRVCL);
status = 502;
goto return_prx_cond;
@@ -1223,10 +1191,7 @@
txn->status = status;
http_reply_and_close(s, txn->status, http_error_message(s));
}
- if (!(s->flags & SF_ERR_MASK))
- s->flags |= SF_ERR_PRXCOND;
- if (!(s->flags & SF_FINST_MASK))
- s->flags |= ((txn->rsp.msg_state < HTTP_MSG_ERROR) ? SF_FINST_H : SF_FINST_D);
+ http_set_term_flags(s);
DBG_TRACE_DEVEL("leaving on error ",
STRM_EV_STRM_ANA|STRM_EV_HTTP_ANA|STRM_EV_HTTP_ERR, s, txn);
return 0;
@@ -2126,19 +2091,6 @@
if (htx->flags & HTX_FL_PROCESSING_ERROR)
goto return_int_err;
- if ((res->flags & (CF_READ_ERROR|CF_READ_TIMEOUT|CF_WRITE_ERROR|CF_WRITE_TIMEOUT)) ||
- ((res->flags & CF_SHUTW) && (res->to_forward || co_data(res)))) {
- /* Output closed while we were sending data. We must abort and
- * wake the other side up.
- */
- msg->msg_state = HTTP_MSG_ERROR;
- http_end_response(s);
- http_end_request(s);
- DBG_TRACE_DEVEL("leaving on error",
- STRM_EV_STRM_ANA|STRM_EV_HTTP_ANA|STRM_EV_HTTP_ERR, s, txn);
- return 1;
- }
-
if (msg->msg_state == HTTP_MSG_BODY)
msg->msg_state = HTTP_MSG_DATA;
@@ -2229,17 +2181,14 @@
channel_dont_close(res);
+ if ((res->flags & CF_SHUTW) && co_data(res)) {
+ /* response errors are most likely due to the client aborting
+ * the transfer. */
+ goto return_cli_abort;
+ }
+
http_end_response(s);
if (!(res->analysers & an_bit)) {
- http_end_request(s);
- if (unlikely(msg->msg_state == HTTP_MSG_ERROR)) {
- if (res->flags & CF_SHUTW) {
- /* response errors are most likely due to the
- * client aborting the transfer. */
- goto return_cli_abort;
- }
- goto return_bad_res;
- }
DBG_TRACE_LEAVE(STRM_EV_STRM_ANA|STRM_EV_HTTP_ANA, s, txn);
return 1;
}
@@ -2298,7 +2247,7 @@
_HA_ATOMIC_INC(&__objt_server(s->target)->counters.srv_aborts);
stream_inc_http_fail_ctr(s);
if (!(s->flags & SF_ERR_MASK))
- s->flags |= SF_ERR_SRVCL;
+ s->flags |= ((res->flags & CF_READ_TIMEOUT) ? SF_ERR_SRVTO : SF_ERR_SRVCL);
goto return_error;
return_cli_abort:
@@ -2309,7 +2258,7 @@
if (objt_server(s->target))
_HA_ATOMIC_INC(&__objt_server(s->target)->counters.cli_aborts);
if (!(s->flags & SF_ERR_MASK))
- s->flags |= SF_ERR_CLICL;
+ s->flags |= ((res->flags & CF_WRITE_TIMEOUT) ? SF_ERR_CLITO : SF_ERR_CLICL);
goto return_error;
return_int_err:
@@ -2337,8 +2286,8 @@
return_error:
/* don't send any error message as we're in the body */
http_reply_and_close(s, txn->status, NULL);
- if (!(s->flags & SF_FINST_MASK))
- s->flags |= SF_FINST_D;
+ http_set_term_flags(s);
+ 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;
@@ -4336,13 +4285,6 @@
DBG_TRACE_ENTER(STRM_EV_HTTP_ANA, s, txn);
- if (unlikely(txn->req.msg_state == HTTP_MSG_ERROR ||
- txn->rsp.msg_state == HTTP_MSG_ERROR)) {
- channel_abort(chn);
- channel_htx_truncate(chn, htxbuf(&chn->buf));
- goto end;
- }
-
if (unlikely(txn->req.msg_state < HTTP_MSG_DONE)) {
DBG_TRACE_DEVEL("waiting end of the request", STRM_EV_HTTP_ANA, s, txn);
return;
@@ -4422,10 +4364,6 @@
txn->req.msg_state = HTTP_MSG_CLOSED;
goto http_msg_closed;
}
- else if (chn->flags & CF_SHUTW) {
- txn->req.msg_state = HTTP_MSG_ERROR;
- goto end;
- }
DBG_TRACE_LEAVE(STRM_EV_HTTP_ANA, s, txn);
return;
}
@@ -4472,13 +4410,6 @@
DBG_TRACE_ENTER(STRM_EV_HTTP_ANA, s, txn);
- if (unlikely(txn->req.msg_state == HTTP_MSG_ERROR ||
- txn->rsp.msg_state == HTTP_MSG_ERROR)) {
- channel_htx_truncate(&s->req, htxbuf(&s->req.buf));
- channel_abort(&s->req);
- goto end;
- }
-
if (unlikely(txn->rsp.msg_state < HTTP_MSG_DONE)) {
DBG_TRACE_DEVEL("waiting end of the response", STRM_EV_HTTP_ANA, s, txn);
return;
@@ -4532,16 +4463,6 @@
txn->rsp.msg_state = HTTP_MSG_CLOSED;
goto http_msg_closed;
}
- else if (chn->flags & CF_SHUTW) {
- txn->rsp.msg_state = HTTP_MSG_ERROR;
- _HA_ATOMIC_INC(&strm_sess(s)->fe->fe_counters.cli_aborts);
- _HA_ATOMIC_INC(&s->be->be_counters.cli_aborts);
- if (strm_sess(s)->listener && strm_sess(s)->listener->counters)
- _HA_ATOMIC_INC(&strm_sess(s)->listener->counters->cli_aborts);
- if (objt_server(s->target))
- _HA_ATOMIC_INC(&__objt_server(s->target)->counters.cli_aborts);
- goto end;
- }
DBG_TRACE_LEAVE(STRM_EV_HTTP_ANA, s, txn);
return;
}