Mailing-List: contact dev-help@httpd.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@httpd.apache.org
Received-SPF: pass (athena.apache.org: local policy includes SPF record at
 spf.trusted-forwarder.org)
Content-Type: text/plain; charset=windows-1252
Mime-Version: 1.0 (Mac OS X Mail 7.1 \(1827\))
Subject: Re: mod_proxy,
 oooled backend connections and the keep-alive race condition
From: Jim Jagielski <jim@jaguNET.com>
In-Reply-To: 
 <CAKQ1sVN77U_dHBZqRYj_1jnXur84pZwx21LO8FdVuU=wXqPkSg@mail.gmail.com>
Date: Thu, 5 Dec 2013 10:05:32 -0500
Content-Transfer-Encoding: quoted-printable
Message-Id: <6E8DB969-9CCD-4AD4-9265-42B510C7AC5C@jaguNET.com>
References: 
 <CAPV0b04sMMdUauv6mK_ttMNf6FDYT+KOswuS5HieAcXjCiFu1g@mail.gmail.com>
 <D87E9FE4-EC89-441A-A38E-DC7A325FA7A1@jaguNET.com>
 <AB1691BE05AE7F4992697F2A0835627A66C5123E@VOEXM10W.internal.vodafone.com>
 <20131001122239.GA28849@redhat.com>
 <CAKQ1sVMvW8BFSiXYF0DG4=_30X6neEV5x3+ZfeYqK_qtG57nEQ@mail.gmail.com>
 <CAPV0b04LDdKzevi-zYsD7=_s9pyd9tmteqV0q1NJ_Dxt+h7atw@mail.gmail.com>
 <CAKQ1sVNrJv5LuPG976Zb0UbF3WRxfVTvjfFMhmQHa15zQMa85g@mail.gmail.com>
 <52A076E5.2080906@redhat.com>
 <CAKQ1sVN77U_dHBZqRYj_1jnXur84pZwx21LO8FdVuU=wXqPkSg@mail.gmail.com>
To: dev@httpd.apache.org

There hardly seemed any consensus on the patch... It also
seems that it adds more cycles to Apache on the front to
reduce a race condition that can't really be removed.
IMO, a reverse proxy should get out of the way as
quickly as possible.

Plus, if we do this here, shouldn't we do it for
all proxy protocol submodules (fcgi, etc...)?

Plus, as mentioned, this single patch includes 2 "fixes"
which can be/should be independent.

On Dec 5, 2013, at 8:06 AM, Yann Ylavic <ylavic.dev@gmail.com> wrote:

> Hi Jan,
>=20
> I don't think it is fixed in trunk, but I may have missed the commits.
> Which ones are you talking about?
>=20
> Regards,
> Yann.
>=20
>=20
> On Thu, Dec 5, 2013 at 1:51 PM, Jan Kalu=9Ea <jkaluza@redhat.com> =
wrote:
> Hi,
>=20
> should this be fixed in trunk already? I see some commits in proxy =
code based on your ideas Yann, but I'm not sure if they address this =
particular problem too.
>=20
> Jan Kaluza
>=20
>=20
> On 10/17/2013 04:52 PM, Yann Ylavic wrote:
>=20
> On Thu, Oct 17, 2013 at 11:36 AM, Thomas Eckert
> <thomas.r.w.eckert@gmail.com <mailto:thomas.r.w.eckert@gmail.com>> =
wrote:
>=20
>      > Hence,
>     why cannot mod_proxy_http prefetch the client's body *before* =
trying
>     anything with the backend connection, and only if it's all good,
>     connect (or reuse a connection to) the
>      > backend and then start forwarding the data immediatly (flushing
>     the prefetched ones) ?
>=20
>     Now I'm confused. Are we talking about flushing everything
>     immediately from client to backend (that's what I understood from
>     your first patch proposal in this thread) or are we talking about
>     pre fetching the whole request body from the client and then =
passing
>     it on to the backend as a whole ?
>=20
>=20
> Sorry, I really should not have proposed 2 patches in the same thread
> without being clear about their different purposes.
>=20
> I first thought flushing everything received from the client to the
> backend immediatly, and disabling *retries* in the 16K-prefetch, would
> address the issue (my first patch,
> "httpd-trunk-mod_proxy_http_flushall.patch"). But as you said in a
> previous message, that did not address the case where the client =
and/or
> an input filter retain the very first body bytes, and mod_proxy_http
> blocks, and the backend timeouts...
>=20
> Hence the second patch ("httpd-trunk-mod_proxy_http_prefetch.patch"),
> which does prefetch *before* establishing/checking the backend's
> connection, so to minimize (to the minimum) the future delay between
> that and the first bytes sent to the backend (hence the chances for it
> to timeout). The prefetch is still up to 16K (same as existing), and =
not
> the "whole" body of course.
>=20
> Then, when it's time to forward (what is available from) the client's
> request, the patched mod_proxy_http will establish or check+reuse a
> backend connection and always flush immediatly what it has  already
> (headers + prefetch, see stream_reqbody_* functions).
>=20
> For the remaining request's body chunks (if any), I intentionally did
> not change the current behaviour of letting the output filters choose
> when to flush, this is not the keep-alive timeout anymore, so it =
becomes
> out of the issue's scope. But I'm definitively +1 to flush-all here =
too
> (optionaly), since mod_proxy may retain small chunks and can then face
> another backend timeout later! That's where patch1 meets patch2, and
> comes patch3 ("httpd-trunk-mod_proxy_http_prefetch+flushall.patch",
> attached here).
>=20
> There is still the potential (minimal) race condition, the backend =
could
> have closed in the (short) meantime, that's indeed *not* addressed by =
my
> patch(es).
>=20
>=20
>     The problem I mentioned in the first message is about treating the
>     backend connection in an error prone way. By not ensuring the
>     connection is still valid - e.g. as seen by the peer - we risk
>     running into this problem no matter what we do elsewhere. So I
>     really think the proper way to handle this is to reopen the
>     connection if necessary - be this with a filter or integrated into
>     the connection handling itself - and only fail after we tried at
>     least once.
>=20
>=20
> IMHO this is hardly feasible for non-idempotent (and no body) requests
> (without disabling connection reuse, or using =
"proxy-initial-not-pooled"
> to let the client choose), even if mod_proxy takes care of not sending
> the full request in one time to be able to detect connection problems
> before it's too late (to retry), there is still a possibility that it
> won't know before sending the last chunk, and the issue remain.
> mod_proxy_http addresses retries for requests *with body* by the =
"ping"
> parameter, maybe it could also retry idempotent requests (like
> mod_proxy_ajp does), but this is still not an overall solution, is =
there
> one?
>=20
>=20
>     Re-thinking the proposed flush mechanism, I don't think using the
>     flushing will really solve the problem, albeit it probably =
narrowing
>     down the window of opportunity for the problem significantly in =
most
>     scenarios. I mention this because I'm getting the feeling two
>     different discussions are being mixed up: Solving the
>     "keep-alive time out" problem and introducing the
>     "flush-all-immediately" option. While the latter might improve the
>     situation with the first, it is no complete solution and there are =
a
>     lot of scenarios where it does not apply due to other factors like
>     filters (as discussed previously).
>=20
>=20
> See above, the flushall does not resolve the issue (filters...), but =
the
> early prefetch + flushall reduces it considerably, IMHO.
>=20
>=20
>     The flush option itself sounds like a good idea, so I see no =
reason
>     why not to put it in. I just don't want to loose focus on the =
actual
>     problem ;-)
>=20
>=20
> I don't see either...
>=20
> Please consider the new (yet) patch attached, can you still encounter
> some "proxy: error reading status line from remote server" with it?
> Even without "proxy-flushall" set, you shouldn't, whatever the time
> taken to read the request first 16K bytes...
>=20
> Regards,
> Yann.
>=20
>=20