You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Yann Ylavic <yl...@gmail.com> on 2014/04/04 19:02:02 UTC
Re: mod_proxy ping and 100-continue (was Re: NOTE: Intent to T&R
2.2.6 tomorrow)
Hi,
this is the day of resurrections :p
I think I've got a simpler way to address this issue, that is, don't
send unexpected 100-continue to clients due to proxy ping feature.
Here is the patch.
Once again, please object if you don't want me to commit this stuff.
Reagrds,
Yann.
Index: modules/proxy/proxy_util.c
===================================================================
--- modules/proxy/proxy_util.c (revision 1584652)
+++ modules/proxy/proxy_util.c (working copy)
@@ -3312,8 +3312,43 @@ PROXY_DECLARE(int) ap_proxy_create_hdrbrgd(apr_poo
* to backend
*/
if (do_100_continue) {
- apr_table_mergen(r->headers_in, "Expect", "100-Continue");
- r->expecting_100 = 1;
+ const char *val;
+
+ /* Since we may modify the original "proxy-interim-response" env but
+ * also come back here later with the same request (previous ping
+ * failure, load balancing recoverable error...), we use "\n" as the
+ * saved value when the original one is not defined, so that we can do
+ * the right thing (unset) to restore the env (later) in this case.
+ */
+ val = apr_table_get(r->notes, "proxy-interim-pong");
+ if (val == NULL) {
+ val = apr_table_get(r->subprocess_env, "proxy-interim-response");
+ if (val != NULL) {
+ apr_table_setn(r->notes, "proxy-interim-pong", val);
+ }
+ else {
+ apr_table_setn(r->notes, "proxy-interim-pong", "\n");
+ }
+ }
+ if (!r->expecting_100) {
+ /* Don't forward any "100 Continue" response if the client is
+ * not expecting it. */
+ val = "Suppress";
+ }
+ if (val && (val[0] != '\n' || val[1] != '\0')) {
+ apr_table_setn(r->subprocess_env, "proxy-interim-response", val);
+ }
+ else {
+ apr_table_unset(r->subprocess_env, "proxy-interim-response");
+ }
+
+ /* Add the Expect header if not already there. */
+ val = apr_table_get(r->headers_in, "Expect");
+ if ((val == NULL)
+ || (strcasecmp(val, "100-Continue") != 0 // fast path
+ && !ap_find_token(r->pool, val, "100-Continue"))) {
+ apr_table_mergen(r->headers_in, "Expect", "100-Continue");
+ }
}
/* X-Forwarded-*: handling
[EOS]
Re: mod_proxy ping and 100-continue (was Re: NOTE: Intent to T&R
2.2.6 tomorrow)
Posted by "William A. Rowe Jr." <wm...@gmail.com>.
This is a once-per-request query, so a note shouldn't be a bad thing.
But I'm wondering if we need a multi-state (and eventually, fold that
into 2.6/3.0 req_req instead)?
Many users have requested that mod_proxy honor -configured- proxypass
backends' 100 responses and defer the 100 response to the client until
the backend is known to be alive.
For lots of reasons, this shouldn't be the default behavior, notably
the lag between the proxy and backend servers. But for an integrated
app hosting environment, the lag should be minimal and the stress on
httpd of reading-to-discard large post/upload sorts of requests when
auth is required or some other fatal error is encountered would be
worthwhile.
So we have the typical httpd-originated 100-continue vs. a
backend-originated 100-continue, and I'm not sure how the delta
between current-default and rfc behavior might factor into a third
mode.
On Fri, Apr 4, 2014 at 5:19 PM, Yann Ylavic <yl...@gmail.com> wrote:
> On Fri, Apr 4, 2014 at 7:52 PM, Jim Jagielski <ji...@jagunet.com> wrote:
>> Is there any way to accomplish w/o using notes? It's not that
>> they are especially slow, it's just that they aren't that fast
>> and, iirc, this could be a tight path.
>
> There surely is, but we can't use the proxy_conn_rec for this (it may
> change between calls for the load-balancing case).
> We could define ap_proxy_create_hdrbrgd_ex() which takes the saved
> "proxy-interim-response" env as argument (looked up once by the
> caller), or add a new request_rec field (...), but I think it's a bit
> (at least) overkill for a path where, anyway, expecting (and probably
> waiting for) a 100-continue will take much more time than a few
> apr_table ops.
>
> The simpler solution, IMHO, is to simply never forward 100-continue
> responses, the client has already got one when this happens.
> I don't really understand the purpose of "proxy-interim-response" ==
> "RFC" anyhow ("suppress" should be the only behaviour), I even think
> it is not RFC compliant (at least useless) to send multiple
> 100-continue...
>
> As said earlier, by pre-fetching the request body, mod_proxy can't be
> an expectation proxy: it should comply to the client part of the RFC
> wrt to the origin (which is the case), and to the server part of the
> RFC wrt the client (which "proxy-interim-response" breaks).
Re: mod_proxy ping and 100-continue (was Re: NOTE: Intent to T&R
2.2.6 tomorrow)
Posted by Yann Ylavic <yl...@gmail.com>.
On Fri, Apr 4, 2014 at 7:52 PM, Jim Jagielski <ji...@jagunet.com> wrote:
> Is there any way to accomplish w/o using notes? It's not that
> they are especially slow, it's just that they aren't that fast
> and, iirc, this could be a tight path.
There surely is, but we can't use the proxy_conn_rec for this (it may
change between calls for the load-balancing case).
We could define ap_proxy_create_hdrbrgd_ex() which takes the saved
"proxy-interim-response" env as argument (looked up once by the
caller), or add a new request_rec field (...), but I think it's a bit
(at least) overkill for a path where, anyway, expecting (and probably
waiting for) a 100-continue will take much more time than a few
apr_table ops.
The simpler solution, IMHO, is to simply never forward 100-continue
responses, the client has already got one when this happens.
I don't really understand the purpose of "proxy-interim-response" ==
"RFC" anyhow ("suppress" should be the only behaviour), I even think
it is not RFC compliant (at least useless) to send multiple
100-continue...
As said earlier, by pre-fetching the request body, mod_proxy can't be
an expectation proxy: it should comply to the client part of the RFC
wrt to the origin (which is the case), and to the server part of the
RFC wrt the client (which "proxy-interim-response" breaks).
Re: mod_proxy ping and 100-continue (was Re: NOTE: Intent to T&R
2.2.6 tomorrow)
Posted by Yann Ylavic <yl...@gmail.com>.
On Fri, Apr 4, 2014 at 7:52 PM, Jim Jagielski <ji...@jagunet.com> wrote:
> Is there any way to accomplish w/o using notes? It's not that
> they are especially slow, it's just that they aren't that fast
> and, iirc, this could be a tight path.
>
Simpler solution commited in r1588519.
We don't have to care about reentrance since r->expecting_100 will
never be 1 in this case.
Re: mod_proxy ping and 100-continue (was Re: NOTE: Intent to T&R 2.2.6 tomorrow)
Posted by Jim Jagielski <ji...@jaguNET.com>.
Is there any way to accomplish w/o using notes? It's not that
they are especially slow, it's just that they aren't that fast
and, iirc, this could be a tight path.
On Apr 4, 2014, at 1:02 PM, Yann Ylavic <yl...@gmail.com> wrote:
> Hi,
>
> this is the day of resurrections :p
>
> I think I've got a simpler way to address this issue, that is, don't
> send unexpected 100-continue to clients due to proxy ping feature.
>
> Here is the patch.
> Once again, please object if you don't want me to commit this stuff.
>
> Reagrds,
> Yann.
>
> Index: modules/proxy/proxy_util.c
> ===================================================================
> --- modules/proxy/proxy_util.c (revision 1584652)
> +++ modules/proxy/proxy_util.c (working copy)
> @@ -3312,8 +3312,43 @@ PROXY_DECLARE(int) ap_proxy_create_hdrbrgd(apr_poo
> * to backend
> */
> if (do_100_continue) {
> - apr_table_mergen(r->headers_in, "Expect", "100-Continue");
> - r->expecting_100 = 1;
> + const char *val;
> +
> + /* Since we may modify the original "proxy-interim-response" env but
> + * also come back here later with the same request (previous ping
> + * failure, load balancing recoverable error...), we use "\n" as the
> + * saved value when the original one is not defined, so that we can do
> + * the right thing (unset) to restore the env (later) in this case.
> + */
> + val = apr_table_get(r->notes, "proxy-interim-pong");
> + if (val == NULL) {
> + val = apr_table_get(r->subprocess_env, "proxy-interim-response");
> + if (val != NULL) {
> + apr_table_setn(r->notes, "proxy-interim-pong", val);
> + }
> + else {
> + apr_table_setn(r->notes, "proxy-interim-pong", "\n");
> + }
> + }
> + if (!r->expecting_100) {
> + /* Don't forward any "100 Continue" response if the client is
> + * not expecting it. */
> + val = "Suppress";
> + }
> + if (val && (val[0] != '\n' || val[1] != '\0')) {
> + apr_table_setn(r->subprocess_env, "proxy-interim-response", val);
> + }
> + else {
> + apr_table_unset(r->subprocess_env, "proxy-interim-response");
> + }
> +
> + /* Add the Expect header if not already there. */
> + val = apr_table_get(r->headers_in, "Expect");
> + if ((val == NULL)
> + || (strcasecmp(val, "100-Continue") != 0 // fast path
> + && !ap_find_token(r->pool, val, "100-Continue"))) {
> + apr_table_mergen(r->headers_in, "Expect", "100-Continue");
> + }
> }
>
> /* X-Forwarded-*: handling
> [EOS]
>