On Tue, Oct 29, 2019 at 8:01 PM Rainer Jung <[email protected]> wrote:
>
> Double check: the condition in the do-while loop that was chaned to a
> while loop has also changed:
>
> FROM
>
> do { ... }
> while ((bytes_read < MAX_MEM_SPOOL - 80)
> && !APR_BUCKET_IS_EOS(APR_BRIGADE_LAST(input_brigade))
> && !req->prefetch_nonblocking);
>
> TO
>
> while (((bytes_read < MAX_MEM_SPOOL - 80)
> && (APR_BRIGADE_EMPTY(input_brigade)
> || !APR_BUCKET_IS_EOS(APR_BRIGADE_LAST(input_brigade))))
> { ... }
>
> That's intended?
Yes, if req->prefetch_nonblocking we need to enter the loop at least
once, but since we are non-blocking it does not hurt to _try_ to read
more (even when we come back here in the balancer fallback case).
There is corner case though, if balancer members don't have the same
Proxy100Continue configuration we may enter ap_proxy_http_prefetch()
with different req->prefetch_nonblocking but mainly req->expecting_100
too. Since we can rely on the HTTP_IN filter to do 100 continue above
the first call, in case where the first balancer member reached is
"Proxy100Continue on" but the fallback is "off" then could deadlock
(similarly to r1868576).
Possibly it's too much of a corner case, but it looks easy enough to
address in this patch (by ignoring "Proxy100Continue off" on the
fallback), so maybe this v2 (attached) instead?
Regards,
Yann.
Index: modules/proxy/mod_proxy_http.c
===================================================================
--- modules/proxy/mod_proxy_http.c (revision 1869076)
+++ modules/proxy/mod_proxy_http.c (working copy)
@@ -648,9 +660,12 @@ static int ap_proxy_http_prefetch(proxy_http_req_t
* us an instant C-L election if the body is of some
* reasonable size.
*/
+ apr_brigade_length(input_brigade, 1, &bytes_read);
temp_brigade = apr_brigade_create(p, bucket_alloc);
block = req->prefetch_nonblocking ? APR_NONBLOCK_READ : APR_BLOCK_READ;
- do {
+ while (bytes_read < MAX_MEM_SPOOL - 80
+ && (APR_BRIGADE_EMPTY(input_brigade)
+ || !APR_BUCKET_IS_EOS(APR_BRIGADE_LAST(input_brigade)))) {
status = ap_get_brigade(r->input_filters, temp_brigade,
AP_MODE_READBYTES, block,
MAX_MEM_SPOOL - bytes_read);
@@ -710,9 +725,7 @@ static int ap_proxy_http_prefetch(proxy_http_req_t
* surrender once we hit 80 bytes less than MAX_MEM_SPOOL
* (an arbitrary value.)
*/
- } while ((bytes_read < MAX_MEM_SPOOL - 80)
- && !APR_BUCKET_IS_EOS(APR_BRIGADE_LAST(input_brigade))
- && !req->prefetch_nonblocking);
+ }
/* Use chunked request body encoding or send a content-length body?
*
@@ -1972,6 +1965,7 @@ static int proxy_http_handler(request_rec *r, prox
const char *u;
proxy_http_req_t *req = NULL;
proxy_conn_rec *backend = NULL;
+ apr_bucket_brigade *input_brigade = NULL;
int is_ssl = 0;
conn_rec *c = r->connection;
proxy_dir_conf *dconf;
@@ -2036,9 +2030,10 @@ static int proxy_http_handler(request_rec *r, prox
req->rb_method = RB_INIT;
dconf = ap_get_module_config(r->per_dir_config, &proxy_module);
+ apr_pool_userdata_get((void **)&input_brigade, "proxy-req-input", p);
/* Should we handle end-to-end or ping 100-continue? */
- if ((r->expecting_100 && dconf->forward_100_continue)
+ if ((r->expecting_100 && (dconf->forward_100_continue || input_brigade))
|| PROXY_DO_100_CONTINUE(worker, r)) {
/* We need to reset r->expecting_100 or prefetching will cause
* ap_http_filter() to send "100 Continue" response by itself. So
@@ -2090,8 +2085,12 @@ static int proxy_http_handler(request_rec *r, prox
* to reduce to the minimum the unavoidable local is_socket_connected() vs
* remote keepalive race condition.
*/
- req->input_brigade = apr_brigade_create(p, req->bucket_alloc);
req->header_brigade = apr_brigade_create(p, req->bucket_alloc);
+ req->input_brigade = input_brigade;
+ if (req->input_brigade == NULL) {
+ req->input_brigade = apr_brigade_create(p, req->bucket_alloc);
+ apr_pool_userdata_setn(req->input_brigade, "proxy-req-input", NULL, p);
+ }
if ((status = ap_proxy_http_prefetch(req, uri, locurl)) != OK)
goto cleanup;
