Hi,

On Wed, Nov 11, 2015 at 03:46:38AM +0000, Laurent Senta wrote:
> Thanks for the reply guys,
> after investigating the source code, it looks like this behavior is wanted.

Yes, it is mandated by the way keep-alive works in HTTP. You never know when
the connection will abort, and at the moment you reuse a connection, it may
fail on you. People don't want to see in their browser errors that are not
real errors and just a result of normal traffic, instead the browser knows
it must retry because that situation is expected. But it will not retry if
it receives a valid response (and an error is a valid response).

> I've been able to "fix it" by removing these two lines:
> 
> diff --git a/src/proto_http.c b/src/proto_http.c
> index 2dcac06..d33b4a1 100644
> --- a/src/proto_http.c
> +++ b/src/proto_http.c
> @@ -6125,8 +6125,6 @@ int http_wait_for_response(struct stream *s, struct
> channel *rep, int an_bit)
>                 else if (rep->flags & CF_READ_TIMEOUT) {
>                         if (msg->err_pos >= 0)
> 
> http_capture_bad_message(&s->be->invalid_rep, s, msg, msg->msg_state,
> sess->fe);
> -                       else if (txn->flags & TX_NOT_FIRST)
> -                               goto abort_keep_alive;

I would argue that we could possibly relax this for timeouts indeed.
Someone who configures haproxy's keep-alive timeout to a value lower
than the surrounding firewalls' timeouts is seeking trouble. And the
bad was already done by making the client wait. So probably we'd
rather report the 504 here.

Would it be enough for everyone if we just removed this one or do we
need something more configurable like Tait's patch ? I think we could
have something with verbosity levels :
  - act the most transparently possible regarding our local issues (for
    browsers), which means replicate on the client side what we see on
    the server side (eg: close when we get a close).
  - maybe report errors in logs but still silently close the connection
  - only report suspicious errors (eg: 504 here) to the client
  - report them all (eg: for webservices where this helps detect a
    failing server)

Before rushing on a patch we should also consider this with the http-reuse
that was introduced in 1.6, to ensure we don't end up with something ugly.

> I traced back that change to:
> http://git.haproxy.org/?p=haproxy-1.6.git;a=commit;h=6b726adb35d998eb55671c0d98ef889cb9fd64ab
> 
> I don't understand why it's saner to kill the connection and hide the 504
> instead of clearly stating the error and let the application handle the
> timeout.

As explained above, it's because a keep-alive enabled client must implement
the ability to replay requests for which it didn't get a response because
the connection died. In fact we're forwarding to the client what we saw on
the server side so that the client can take the correct decision. If your
client was directly connected to the server, it would have seen the exact
same behaviour.

Regards,
Willy


Reply via email to