On Mon, Nov 4, 2019 at 10:06 PM Ruediger Pluem <[email protected]> wrote:
>
> Sorry for getting back late. I missed that the HTTP_IN filter only sends a
> possible
> 100 continue during the first run. Good catch. Nevertheless this leaves the
> question open
> how to solve this. Either by your patch which sends the intermediate response
> itself
> or by fixing HTTP_IN to check for the need to sent it not only on the first
> run.
> I am undecided. Maybe someone else has a strong opinion either way otherwise
> just move on with your patch.
Not someone else but still \o_
Actually I like the idea of HTTP_IN to handle 100 continue for others
cases than the first time around, it greatly simplifies things
(including for mod_proxy_http).
The way I see it is to move 100 continue handling out of the first
time only block, as you propose, and to change the decision for when
it's done like:
/* Since we're about to read data, send 100-Continue if needed.
- * Only valid on chunked and C-L bodies where the C-L is > 0. */
- if ((ctx->state == BODY_CHUNK
- || (ctx->state == BODY_LENGTH && ctx->remaining > 0))
+ * Only valid on chunked and C-L bodies where the C-L is > 0.
+ *
+ * If the read is to be nonblocking though, the caller may not want to
+ * handle this just now (e.g. mod_proxy_http), and is prepared to read
+ * nothing should the client really waits for 100 continue, so we can
+ * avoid sending it now and wait for the next blocking read.
+ *
+ * Also, we track in ctx->data_read whether something is read below,
+ * which allows to not send 100 continue either in this case, according
+ * to https://tools.ietf.org/html/rfc7231#section-5.1.1 :
+ * A server MAY omit sending a 100 (Continue) response if it has
+ * already received some or all of the message body for the
+ * corresponding request, ...
+ *
+ * In any case, even if r->expecting remains set at the end of the
+ * request processing, ap_set_keepalive() will finally do the right
+ * thing (i.e. "Connection: close" the connection).
+ */
+ if (block == APR_BLOCK_READ
+ && (ctx->state == BODY_CHUNK
+ || (ctx->state == BODY_LENGTH && ctx->remaining > 0))
&& f->r->expecting_100 && f->r->proto_num >= HTTP_VERSION(1,1)
- && !(f->r->eos_sent || f->r->bytes_sent)) {
+ && !(ctx->data_read || f->r->eos_sent || f->r->bytes_sent)) {
Full patch attached, changes for both HTTP_IN and mod_proxy_http (this
is a POC only, untested yet).
I like the way it removes all the r->100_continue => req->100_continue
=> r->100_continue danse in mod_proxy_http (since HTTP_IN now does
what it wants in nonblocking mode), while the changes in HTTP_IN do
not look to insane (note the use of ap_send_interim_response() instead
of open coding it)...
WDYT?
Regards,
Yann.
Index: modules/http/http_filters.c
===================================================================
--- modules/http/http_filters.c (revision 1869400)
+++ modules/http/http_filters.c (working copy)
@@ -79,7 +79,8 @@ typedef struct http_filter_ctx
BODY_CHUNK_END_LF, /* got CR after data, expect LF */
BODY_CHUNK_TRAILER /* trailers */
} state;
- unsigned int eos_sent :1;
+ unsigned int eos_sent :1,
+ data_read:1;
} http_ctx_t;
/**
@@ -401,47 +402,47 @@ apr_status_t ap_http_filter(ap_filter_t *f, apr_bu
* proxied *response*, proxy responses MUST be exempt.
*/
if (ctx->state == BODY_NONE && f->r->proxyreq != PROXYREQ_RESPONSE) {
- e = apr_bucket_eos_create(f->c->bucket_alloc);
- APR_BRIGADE_INSERT_TAIL(b, e);
- ctx->eos_sent = 1;
- return APR_SUCCESS;
+ ctx->eos_sent = 1; /* send EOS below */
}
+ }
+ {
/* Since we're about to read data, send 100-Continue if needed.
- * Only valid on chunked and C-L bodies where the C-L is > 0. */
- if ((ctx->state == BODY_CHUNK
- || (ctx->state == BODY_LENGTH && ctx->remaining > 0))
+ * Only valid on chunked and C-L bodies where the C-L is > 0.
+ *
+ * If the read is to be nonblocking though, the caller may not want to
+ * handle this just now (e.g. mod_proxy_http), and is prepared to read
+ * nothing should the client really waits for 100 continue, so we can
+ * avoid sending it now and wait for the next blocking read.
+ *
+ * Also, we track in ctx->data_read whether something is read below,
+ * which allows to not send 100 continue either in this case, according
+ * to https://tools.ietf.org/html/rfc7231#section-5.1.1 :
+ * A server MAY omit sending a 100 (Continue) response if it has
+ * already received some or all of the message body for the
+ * corresponding request, ...
+ *
+ * In any case, even if r->expecting remains set at the end of the
+ * request processing, ap_set_keepalive() will finally do the right
+ * thing (i.e. "Connection: close" the connection).
+ */
+ if (block == APR_BLOCK_READ
+ && (ctx->state == BODY_CHUNK
+ || (ctx->state == BODY_LENGTH && ctx->remaining > 0))
&& f->r->expecting_100 && f->r->proto_num >= HTTP_VERSION(1,1)
- && !(f->r->eos_sent || f->r->bytes_sent)) {
+ && !(ctx->data_read || f->r->eos_sent || f->r->bytes_sent)) {
if (!ap_is_HTTP_SUCCESS(f->r->status)) {
ctx->state = BODY_NONE;
- ctx->eos_sent = 1;
+ ctx->eos_sent = 1; /* send EOS below */
}
else {
- char *tmp;
- int len;
- apr_bucket_brigade *bb;
+ int saved_status = f->r->status;
- bb = apr_brigade_create(f->r->pool, f->c->bucket_alloc);
+ f->r->status = HTTP_CONTINUE;
+ ap_send_interim_response(f->r, 0);
+ AP_DEBUG_ASSERT(!f->r->expecting_100);
- /* if we send an interim response, we're no longer
- * in a state of expecting one.
- */
- f->r->expecting_100 = 0;
- tmp = apr_pstrcat(f->r->pool, AP_SERVER_PROTOCOL " ",
- ap_get_status_line(HTTP_CONTINUE), CRLF CRLF, NULL);
- len = strlen(tmp);
- ap_xlate_proto_to_ascii(tmp, len);
- e = apr_bucket_pool_create(tmp, len, f->r->pool,
- f->c->bucket_alloc);
- APR_BRIGADE_INSERT_HEAD(bb, e);
- e = apr_bucket_flush_create(f->c->bucket_alloc);
- APR_BRIGADE_INSERT_TAIL(bb, e);
-
- rv = ap_pass_brigade(f->c->output_filters, bb);
- if (rv != APR_SUCCESS) {
- return AP_FILTER_ERROR;
- }
+ f->r->status = saved_status;
}
}
}
@@ -491,8 +492,10 @@ apr_status_t ap_http_filter(ap_filter_t *f, apr_bu
if (!APR_BUCKET_IS_METADATA(e)) {
rv = apr_bucket_read(e, &buffer, &len, APR_BLOCK_READ);
-
if (rv == APR_SUCCESS) {
+ if (len > 0 && !ctx->data_read) {
+ ctx->data_read = 1;
+ }
rv = parse_chunk_size(ctx, buffer, len,
f->r->server->limit_req_fieldsize, strict);
}
@@ -548,6 +551,9 @@ apr_status_t ap_http_filter(ap_filter_t *f, apr_bu
/* How many bytes did we just read? */
apr_brigade_length(b, 0, &totalread);
+ if (totalread > 0 && !ctx->data_read) {
+ ctx->data_read = 1;
+ }
/* If this happens, we have a bucket of unknown length. Die because
* it means our assumptions have changed. */
Index: modules/proxy/mod_proxy_http.c
===================================================================
--- modules/proxy/mod_proxy_http.c (revision 1869400)
+++ modules/proxy/mod_proxy_http.c (working copy)
@@ -254,7 +254,6 @@ typedef struct {
rb_methods rb_method;
- int expecting_100;
unsigned int do_100_continue:1,
prefetch_nonblocking:1;
} proxy_http_req_t;
@@ -434,21 +433,6 @@ static int spool_reqbody_cl(proxy_http_req_t *req,
apr_file_t *tmpfile = NULL;
apr_off_t limit;
- /* Send "100 Continue" now if the client expects one, before
- * blocking on the body, otherwise we'd wait for each other.
- */
- if (req->expecting_100) {
- int saved_status = r->status;
-
- r->expecting_100 = 1;
- r->status = HTTP_CONTINUE;
- ap_send_interim_response(r, 0);
- AP_DEBUG_ASSERT(!r->expecting_100);
-
- r->status = saved_status;
- req->expecting_100 = 0;
- }
-
body_brigade = apr_brigade_create(p, bucket_alloc);
*bytes_spooled = 0;
@@ -579,7 +563,7 @@ static int ap_proxy_http_prefetch(proxy_http_req_t
apr_read_type_e block;
if (apr_table_get(r->subprocess_env, "force-proxy-request-1.0")) {
- if (req->expecting_100) {
+ if (r->expecting_100) {
return HTTP_EXPECTATION_FAILED;
}
force10 = 1;
@@ -684,18 +668,6 @@ static int ap_proxy_http_prefetch(proxy_http_req_t
apr_brigade_length(temp_brigade, 1, &bytes);
bytes_read += bytes;
- /* From https://tools.ietf.org/html/rfc7231#section-5.1.1
- * A server MAY omit sending a 100 (Continue) response if it has
- * already received some or all of the message body for the
- * corresponding request, or if [snip].
- */
- if (req->expecting_100 && bytes > 0) {
- ap_log_rerror(APLOG_MARK, APLOG_DEBUG, status, r, APLOGNO(10206)
- "prefetching request body while 100-continue is"
- "expected, omit sending interim response");
- req->expecting_100 = 0;
- }
-
/*
* Save temp_brigade in input_brigade. (At least) in the SSL case
* temp_brigade contains transient buckets whose data would get
@@ -1513,11 +1485,7 @@ int ap_proxy_http_process_response(proxy_http_req_
if (!policy
|| (!strcasecmp(policy, "RFC")
&& (proxy_status != HTTP_CONTINUE
- || (req->expecting_100 = 1)))) {
- if (proxy_status == HTTP_CONTINUE) {
- r->expecting_100 = req->expecting_100;
- req->expecting_100 = 0;
- }
+ || (r->expecting_100 = 1)))) {
ap_send_interim_response(r, 1);
}
/* FIXME: refine this to be able to specify per-response-status
@@ -1610,8 +1578,6 @@ int ap_proxy_http_process_response(proxy_http_req_
* to do the right thing according to the final response and
* any later update of r->expecting_100.
*/
- r->expecting_100 = req->expecting_100;
- req->expecting_100 = 0;
}
/* Once only! */
@@ -2036,17 +2002,7 @@ static int proxy_http_handler(request_rec *r, prox
/* Should we handle end-to-end or ping 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
- * we'll use req->expecting_100 in mod_proxy_http to determine whether
- * the client should be forwarded "100 continue", and r->expecting_100
- * will be restored at the end of the function with the actual value of
- * req->expecting_100 (i.e. cleared only if mod_proxy_http sent the
- * "100 Continue" according to its policy).
- */
req->do_100_continue = req->prefetch_nonblocking = 1;
- req->expecting_100 = r->expecting_100;
- r->expecting_100 = 0;
}
/* Should we block while prefetching the body or try nonblocking and flush
* data to the backend ASAP?
@@ -2182,10 +2138,6 @@ cleanup:
req->backend->close = 1;
ap_proxy_http_cleanup(proxy_function, r, req->backend);
}
- if (req->expecting_100) {
- /* Restore r->expecting_100 if we didn't touch it */
- r->expecting_100 = req->expecting_100;
- }
return status;
}
