Le 06/07/2017 à 21:18, Christopher Faulet a écrit :
Le 06/07/2017 à 18:56, Lukas Tribus a écrit :
Hi Christopher,
Am 30.06.2017 um 11:14 schrieb Christopher Faulet:
We are seeing this as well on 1.7.5. The problem seems to be intermittent--it
doesn't happen very often when I hit a system with almost no load, but is
happening very frequently on a high load system. I don't believe it is causing
any issues other than the log message.
Hi,
I already proposed a patch to fix this bug in another thread[1]. I have had no
feedback yet. Could you check it please ?
[1] https://www.mail-archive.com/haproxy@formilux.org/msg26543.html
Simon Ellefsene also reported this on discourse:
https://discourse.haproxy.org/t/session-state-sdnn-http-status-code-200-when-upgrading-to-1-7/1409/3
Your patch seems to break responses for him (seems
only headers are returned):
A request that would normally return around 25kb now returns ~300
bytes (just the headers, empty body), status code 200
Hi Lukas,
Thanks. The proposed patched is wrong, I know. In fact, the original bug
is harder to fix than I supposed first. I need more time to fully
understand it. In fact, I know where the problem is, but for now, all my
attempts to fix it break some other parts. It is a real mess and I need
to check everything with many options (forceclose / http-server-close
...). I was so busy on this bug that I forgot to warn everyone. Thanks
to be aware for me :)
I'll try to do my best to proposed a patch very quickly. Then I'll
probably need help to validate it on real traffic.
Hi all,
We finally pushed fixes for this bug in 1.8. I attached a file with all
changes in a single patch for 1.7. It could be nice if anyone can check
if it does not break anything. And for guys who reported the bug, It
could be good to know if it is fixed with this patch.
Because these last weeks, there were several regressions on this part
(the end of the HTTP transaction), I prefer to be careful this time.
Every time I fixed a bug in one side, this broke something else from
another one...
Thanks
--
Christopher Faulet
>From 8748ee8fe994a3d7facd21ec39d209e894c14510 Mon Sep 17 00:00:00 2001
From: Christopher Faulet <cfau...@haproxy.com>
Date: Tue, 18 Jul 2017 11:18:46 +0200
Subject: [PATCH] BUG: Fix bug about wrong termination state on some requests
introduced in 1.7.5
This pacth contains following fixes coming from the upstream.
- * -
MINOR: http: Reorder/rewrite checks in http_resync_states
The previous patch removed the forced symmetry of the TUNNEL mode during the
state synchronization. Here, we take care to remove body analyzer only on the
channel in TUNNEL mode. In fact, today, this change has no effect because both
sides are switched in same time. But this way, with some changes, it will be
possible to keep body analyzer on a side (to finish the states synchronization)
with the other one in TUNNEL mode.
WARNING: This patch will be used to fix a bug. The fix will be commited in a
very next commit. So if the fix is backported, this one must be backported too.
- * -
MINOR: http: Switch requests/responses in TUNNEL mode only by checking txn flags
Today, the only way to have a request or a response in HTTP_MSG_TUNNEL state is
to have the flag TX_CON_WANT_TUN set on the transaction. So this is a symmetric
state. Both the request and the response are switch in same time in this
state. This can be done only by checking transaction flags instead of relying on
the other side state. This is the purpose of this patch.
This way, if for any reason we need to switch only one side in TUNNEL mode, it
will be possible. And to prepare asymmetric cases, we check channel flags in
DONE _AND_ TUNNEL states.
WARNING: This patch will be used to fix a bug. The fix will be commited in a
very next commit. So if the fix is backported, this one must be backported too.
- * -
BUG/MEDIUM: http: Switch HTTP responses in TUNNEL mode when body length is undefined
When the body length of a HTTP response is undefined, the HTTP parser is blocked
in the body parsing. Before HAProxy 1.7, in this case, because
AN_RES_HTTP_XFER_BODY is never set, there is no visible effect. When the server
closes its connection to terminate the response, HAProxy catches it as a normal
closure. Since 1.7, we always set this analyzer to enter at least once in
http_response_forward_body. But, in the present case, when the server connection
is closed, http_response_forward_body is called one time too many. The response
is correctly sent to the client, but an error is catched and logged with "SD--"
flags.
To reproduce the bug, you can use the configuration "tests/test-fsm.cfg". The
tests 3 and 21 hit the bug.
Idea to fix the bug is to switch the response in TUNNEL mode without switching
the request. This is possible because of previous patches.
First, we need to detect responses with undefined body length during states
synchronization. Excluding tunnelled transactions, when the response length is
undefined, TX_CON_WANT_CLO is always set on the transaction. So, when states are
synchronized, if TX_CON_WANT_CLO is set, the response is switched in TUNNEL mode
and the request remains unchanged.
Then, in http_msg_forward_body, we add a specific check to switch the response
in DONE mode if the body length is undefined and if there is no data filter.
This patch depends on following previous commits:
* MINOR: http: Switch requests/responses in TUNNEL mode only by checking txn flags
* MINOR: http: Reorder/rewrite checks in http_resync_states
This patch must be backported in 1.7 with 2 previous ones.
---
src/proto_http.c | 150 ++++++++++++++++++++++++++++---------------------------
1 file changed, 76 insertions(+), 74 deletions(-)
diff --git a/src/proto_http.c b/src/proto_http.c
index 94c8d639..3def9269 100644
--- a/src/proto_http.c
+++ b/src/proto_http.c
@@ -5294,7 +5294,7 @@ int http_sync_req_state(struct stream *s)
unsigned int old_flags = chn->flags;
unsigned int old_state = txn->req.msg_state;
- if (unlikely(txn->req.msg_state < HTTP_MSG_BODY))
+ if (unlikely(txn->req.msg_state < HTTP_MSG_DONE))
return 0;
if (txn->req.msg_state == HTTP_MSG_DONE) {
@@ -5338,13 +5338,6 @@ int http_sync_req_state(struct stream *s)
goto wait_other_side;
}
- if (txn->rsp.msg_state == HTTP_MSG_TUNNEL) {
- /* if any side switches to tunnel mode, the other one does too */
- channel_auto_read(chn);
- txn->req.msg_state = HTTP_MSG_TUNNEL;
- goto wait_other_side;
- }
-
/* When we get here, it means that both the request and the
* response have finished receiving. Depending on the connection
* mode, we'll have to wait for the last bytes to leave in either
@@ -5361,7 +5354,16 @@ int http_sync_req_state(struct stream *s)
* let's enforce it now that we're not expecting any new
* data to come. The caller knows the stream is complete
* once both states are CLOSED.
+ *
+ * However, there is an exception if the response
+ * length is undefined. In this case, we need to wait
+ * the close from the server. The response will be
+ * switched in TUNNEL mode until the end.
*/
+ if (!(txn->rsp.flags & HTTP_MSGF_XFER_LEN) &&
+ txn->rsp.msg_state != HTTP_MSG_CLOSED)
+ goto check_channel_flags;
+
if (!(chn->flags & (CF_SHUTW|CF_SHUTW_NOW))) {
channel_shutr_now(chn);
channel_shutw_now(chn);
@@ -5377,20 +5379,7 @@ int http_sync_req_state(struct stream *s)
}
}
- if (chn->flags & (CF_SHUTW|CF_SHUTW_NOW)) {
- /* if we've just closed an output, let's switch */
- s->si[1].flags |= SI_FL_NOLINGER; /* we want to close ASAP */
-
- if (!channel_is_empty(chn)) {
- txn->req.msg_state = HTTP_MSG_CLOSING;
- goto http_msg_closing;
- }
- else {
- txn->req.msg_state = HTTP_MSG_CLOSED;
- goto http_msg_closed;
- }
- }
- goto wait_other_side;
+ goto check_channel_flags;
}
if (txn->req.msg_state == HTTP_MSG_CLOSING) {
@@ -5419,6 +5408,16 @@ int http_sync_req_state(struct stream *s)
goto wait_other_side;
}
+ check_channel_flags:
+ /* Here, we are in HTTP_MSG_DONE or HTTP_MSG_TUNNEL */
+ if (chn->flags & (CF_SHUTW|CF_SHUTW_NOW)) {
+ /* if we've just closed an output, let's switch */
+ s->si[1].flags |= SI_FL_NOLINGER; /* we want to close ASAP */
+ txn->req.msg_state = HTTP_MSG_CLOSING;
+ goto http_msg_closing;
+ }
+
+
wait_other_side:
return txn->req.msg_state != old_state || chn->flags != old_flags;
}
@@ -5438,7 +5437,7 @@ int http_sync_res_state(struct stream *s)
unsigned int old_flags = chn->flags;
unsigned int old_state = txn->rsp.msg_state;
- if (unlikely(txn->rsp.msg_state < HTTP_MSG_BODY))
+ if (unlikely(txn->rsp.msg_state < HTTP_MSG_DONE))
return 0;
if (txn->rsp.msg_state == HTTP_MSG_DONE) {
@@ -5461,14 +5460,6 @@ int http_sync_res_state(struct stream *s)
goto wait_other_side;
}
- if (txn->req.msg_state == HTTP_MSG_TUNNEL) {
- /* if any side switches to tunnel mode, the other one does too */
- channel_auto_read(chn);
- txn->rsp.msg_state = HTTP_MSG_TUNNEL;
- chn->flags |= CF_NEVER_WAIT;
- goto wait_other_side;
- }
-
/* When we get here, it means that both the request and the
* response have finished receiving. Depending on the connection
* mode, we'll have to wait for the last bytes to leave in either
@@ -5489,8 +5480,16 @@ int http_sync_res_state(struct stream *s)
* let's enforce it now that we're not expecting any new
* data to come. The caller knows the stream is complete
* once both states are CLOSED.
+ *
+ * However, there is an exception if the response length
+ * is undefined. In this case, we switch in TUNNEL mode.
*/
- if (!(chn->flags & (CF_SHUTW|CF_SHUTW_NOW))) {
+ if (!(txn->rsp.flags & HTTP_MSGF_XFER_LEN)) {
+ channel_auto_read(chn);
+ txn->rsp.msg_state = HTTP_MSG_TUNNEL;
+ chn->flags |= CF_NEVER_WAIT;
+ }
+ else if (!(chn->flags & (CF_SHUTW|CF_SHUTW_NOW))) {
channel_shutr_now(chn);
channel_shutw_now(chn);
}
@@ -5506,18 +5505,7 @@ int http_sync_res_state(struct stream *s)
txn->rsp.msg_state = HTTP_MSG_TUNNEL;
}
- if (chn->flags & (CF_SHUTW|CF_SHUTW_NOW)) {
- /* if we've just closed an output, let's switch */
- if (!channel_is_empty(chn)) {
- txn->rsp.msg_state = HTTP_MSG_CLOSING;
- goto http_msg_closing;
- }
- else {
- txn->rsp.msg_state = HTTP_MSG_CLOSED;
- goto http_msg_closed;
- }
- }
- goto wait_other_side;
+ goto check_channel_flags;
}
if (txn->rsp.msg_state == HTTP_MSG_CLOSING) {
@@ -5548,6 +5536,14 @@ int http_sync_res_state(struct stream *s)
goto wait_other_side;
}
+ check_channel_flags:
+ /* Here, we are in HTTP_MSG_DONE or HTTP_MSG_TUNNEL */
+ if (chn->flags & (CF_SHUTW|CF_SHUTW_NOW)) {
+ /* if we've just closed an output, let's switch */
+ txn->rsp.msg_state = HTTP_MSG_CLOSING;
+ goto http_msg_closing;
+ }
+
wait_other_side:
/* We force the response to leave immediately if we're waiting for the
* other side, since there is no pending shutdown to push it out.
@@ -5577,34 +5573,27 @@ int http_resync_states(struct stream *s)
/* OK, both state machines agree on a compatible state.
* There are a few cases we're interested in :
- * - HTTP_MSG_TUNNEL on either means we have to disable both analysers
* - HTTP_MSG_CLOSED on both sides means we've reached the end in both
* directions, so let's simply disable both analysers.
- * - HTTP_MSG_CLOSED on the response only means we must abort the
- * request.
- * - HTTP_MSG_CLOSED on the request and HTTP_MSG_DONE on the response
- * with server-close mode means we've completed one request and we
- * must re-initialize the server connection.
+ * - HTTP_MSG_CLOSED on the response only or HTTP_MSG_ERROR on either
+ * means we must abort the request.
+ * - HTTP_MSG_TUNNEL on either means we have to disable analyser on
+ * corresponding channel.
+ * - HTTP_MSG_DONE or HTTP_MSG_CLOSED on the request and HTTP_MSG_DONE
+ * on the response with server-close mode means we've completed one
+ * request and we must re-initialize the server connection.
*/
-
- if (txn->req.msg_state == HTTP_MSG_TUNNEL ||
- txn->rsp.msg_state == HTTP_MSG_TUNNEL ||
- (txn->req.msg_state == HTTP_MSG_CLOSED &&
- txn->rsp.msg_state == HTTP_MSG_CLOSED)) {
+ if (txn->req.msg_state == HTTP_MSG_CLOSED &&
+ txn->rsp.msg_state == HTTP_MSG_CLOSED) {
s->req.analysers &= AN_REQ_FLT_END;
channel_auto_close(&s->req);
channel_auto_read(&s->req);
s->res.analysers &= AN_RES_FLT_END;
channel_auto_close(&s->res);
channel_auto_read(&s->res);
- if (txn->req.msg_state == HTTP_MSG_TUNNEL && HAS_REQ_DATA_FILTERS(s))
- s->req.analysers |= AN_REQ_FLT_XFER_DATA;
- if (txn->rsp.msg_state == HTTP_MSG_TUNNEL && HAS_RSP_DATA_FILTERS(s))
- s->res.analysers |= AN_RES_FLT_XFER_DATA;
- }
- else if ((txn->req.msg_state >= HTTP_MSG_DONE &&
- (txn->rsp.msg_state == HTTP_MSG_CLOSED || (s->res.flags & CF_SHUTW))) ||
- txn->rsp.msg_state == HTTP_MSG_ERROR ||
+ }
+ else if (txn->rsp.msg_state == HTTP_MSG_CLOSED ||
+ txn->rsp.msg_state == HTTP_MSG_ERROR ||
txn->req.msg_state == HTTP_MSG_ERROR) {
s->res.analysers &= AN_RES_FLT_END;
channel_auto_close(&s->res);
@@ -5615,6 +5604,23 @@ int http_resync_states(struct stream *s)
channel_auto_read(&s->req);
channel_truncate(&s->req);
}
+ else if (txn->req.msg_state == HTTP_MSG_TUNNEL ||
+ txn->rsp.msg_state == HTTP_MSG_TUNNEL) {
+ if (txn->req.msg_state == HTTP_MSG_TUNNEL) {
+ s->req.analysers &= AN_REQ_FLT_END;
+ if (HAS_REQ_DATA_FILTERS(s))
+ s->req.analysers |= AN_REQ_FLT_XFER_DATA;
+ }
+ if (txn->rsp.msg_state == HTTP_MSG_TUNNEL) {
+ s->res.analysers &= AN_RES_FLT_END;
+ if (HAS_RSP_DATA_FILTERS(s))
+ s->res.analysers |= AN_RES_FLT_XFER_DATA;
+ }
+ channel_auto_close(&s->req);
+ channel_auto_read(&s->req);
+ channel_auto_close(&s->res);
+ channel_auto_read(&s->res);
+ }
else if ((txn->req.msg_state == HTTP_MSG_DONE ||
txn->req.msg_state == HTTP_MSG_CLOSED) &&
txn->rsp.msg_state == HTTP_MSG_DONE &&
@@ -6963,14 +6969,6 @@ int http_response_forward_body(struct stream *s, struct channel *res, int an_bit
if ((msg->flags & HTTP_MSGF_TE_CHNK) || (msg->flags & HTTP_MSGF_COMPRESSING))
res->flags |= CF_EXPECT_MORE;
- /* If there is neither content-length, nor transfer-encoding header
- * _AND_ there is no data filtering, we can safely forward all data
- * indefinitely. */
- if (!(msg->flags & HTTP_MSGF_XFER_LEN) && !HAS_DATA_FILTERS(s, res)) {
- buffer_flush(res->buf);
- channel_forward_forever(res);
- }
-
/* the stream handler will take care of timeouts and errors */
return 0;
@@ -7047,9 +7045,13 @@ http_msg_forward_body(struct stream *s, struct http_msg *msg)
goto missing_data_or_waiting;
}
- /* The server still sending data that should be filtered */
- if (!(msg->flags & HTTP_MSGF_XFER_LEN) && !(chn->flags & CF_SHUTR))
- goto missing_data_or_waiting;
+ /* This check can only be true for a response. HTTP_MSGF_XFER_LEN is
+ * always set for a request. */
+ if (!(msg->flags & HTTP_MSGF_XFER_LEN)) {
+ /* The server still sending data that should be filtered */
+ if (!(chn->flags & CF_SHUTR) && HAS_DATA_FILTERS(s, chn))
+ goto missing_data_or_waiting;
+ }
msg->msg_state = HTTP_MSG_ENDING;
--
2.13.3
