On Thu, Sep 1, 2016 at 10:14 AM, Yann Ylavic <[email protected]> wrote:
> On Thu, Sep 1, 2016 at 9:57 AM, Yann Ylavic <[email protected]> wrote:
>>
>> I'm not sure the readahead is worth it in 2.2.x though, because
>> check_pipeline_flush() does an inconditional flush anyway, and the
>> issue in 2.4.x was that trailing newline(s) might have caused the next
>> request to block in ap_read_request() without flushing the previous
>> one.
>> AFAICT this can't happen in 2.2.x thanks to the FLUSH.
>
> Hmm, my bad, I missed the early return (/* don't flush */) in
> check_pipeline_flush().
> So we may also either backport the check_pipeline() read-ahead part,
> or simply really always FLUSH (avoiding pipelined response).
> WDYT?
Attached is the patch to sync 2.2.x's check_pipeline_flush() with
2.4.x's check_pipeline(), but actually the original 2.2.x code uses
AP_MODE_EATCRLF (which was somehow replaced by a single
AP_MODE_SPECULATIVE earlier in 2.4.x), so it already achieves the same
(modulo the limit on the number of empty lines).
Not sure it's worth the change then, we shouldn't read indefinitely
only [CR]LF in non-blocking mode, and once we get EAGAIN we'd fall
through read_request_line() which should detect (and stop on) more
ones...
Not a big deal IMHO.
Index: modules/http/http_request.c
===================================================================
--- modules/http/http_request.c (revision 1758764)
+++ modules/http/http_request.c (working copy)
@@ -229,36 +229,110 @@ AP_DECLARE(void) ap_die(int type, request_rec *r)
static void check_pipeline_flush(request_rec *r)
{
- apr_bucket *e;
+ apr_status_t rv;
apr_bucket_brigade *bb;
conn_rec *c = r->connection;
+ int num_blank_lines = DEFAULT_LIMIT_BLANK_LINES;
+ ap_input_mode_t mode = AP_MODE_SPECULATIVE;
+ apr_size_t cr = 0;
+ char buf[2];
+
/* ### if would be nice if we could PEEK without a brigade. that would
### allow us to defer creation of the brigade to when we actually
### need to send a FLUSH. */
bb = apr_brigade_create(r->pool, c->bucket_alloc);
- /* Flush the filter contents if:
- *
- * 1) the connection will be closed
- * 2) there isn't a request ready to be read
- */
- /* ### shouldn't this read from the connection input filters? */
- /* ### is zero correct? that means "read one line" */
- if (r->connection->keepalive != AP_CONN_CLOSE) {
- if (ap_get_brigade(r->input_filters, bb, AP_MODE_EATCRLF,
- APR_NONBLOCK_READ, 0) != APR_SUCCESS) {
- c->data_in_input_filters = 0; /* we got APR_EOF or an error */
+ c->data_in_input_filters = 0;
+ while (c->keepalive != AP_CONN_CLOSE && !c->aborted) {
+ apr_size_t len = cr + 1;
+
+ apr_brigade_cleanup(bb);
+ rv = ap_get_brigade(c->input_filters, bb, mode,
+ APR_NONBLOCK_READ, len);
+ if (rv != APR_SUCCESS || APR_BRIGADE_EMPTY(bb)) {
+ /*
+ * Error or empty brigade: There is no data present in the input
+ * filter
+ */
+ if (mode == AP_MODE_READBYTES) {
+ /* Unexpected error, stop with this connection */
+ ap_log_cerror(APLOG_MARK, APLOG_ERR, rv, c,
+ "Can't consume pipelined empty lines");
+ c->keepalive = AP_CONN_CLOSE;
+ }
+ break;
}
+
+ /* Ignore trailing blank lines (which must not be interpreted as
+ * pipelined requests) up to the limit, otherwise we would block
+ * on the next read without flushing data, and hence possibly delay
+ * pending response(s) until the next/real request comes in or the
+ * keepalive timeout expires.
+ */
+ rv = apr_brigade_flatten(bb, buf, &len);
+ if (rv != APR_SUCCESS || len != cr + 1) {
+ int log_level;
+ if (mode == AP_MODE_READBYTES) {
+ /* Unexpected error, stop with this connection */
+ c->keepalive = AP_CONN_CLOSE;
+ log_level = APLOG_ERR;
+ }
+ else {
+ /* Let outside (non-speculative/blocking) read determine
+ * where this possible failure comes from (metadata,
+ * morphed EOF socket => empty bucket? debug only here).
+ */
+ c->data_in_input_filters = 1;
+ log_level = APLOG_DEBUG;
+ }
+ ap_log_cerror(APLOG_MARK, log_level, rv, c,
+ "Can't check pipelined data");
+ break;
+ }
+
+ if (mode == AP_MODE_READBYTES) {
+ mode = AP_MODE_SPECULATIVE;
+ cr = 0;
+ }
+ else if (cr) {
+ AP_DEBUG_ASSERT(len == 2 && buf[0] == APR_ASCII_CR);
+ if (buf[1] == APR_ASCII_LF) {
+ mode = AP_MODE_READBYTES;
+ num_blank_lines--;
+ }
+ else {
+ c->data_in_input_filters = 1;
+ break;
+ }
+ }
else {
- c->data_in_input_filters = 1;
- return; /* don't flush */
+ if (buf[0] == APR_ASCII_LF) {
+ mode = AP_MODE_READBYTES;
+ num_blank_lines--;
+ }
+ else if (buf[0] == APR_ASCII_CR) {
+ cr = 1;
+ }
+ else {
+ c->data_in_input_filters = 1;
+ break;
+ }
}
+ /* Enough blank lines with this connection?
+ * Stop and don't recycle it.
+ */
+ if (num_blank_lines < 0) {
+ c->keepalive = AP_CONN_CLOSE;
+ }
}
- /* The flush below became unconditional way back in r105919. Later
- * branches have very different handling in this area anyway (EOR bucket).
+ /* Flush the filter contents if:
+ *
+ * 1) the connection will be closed (but not aborted already)
+ * 2) there isn't a request ready to be read
*/
-
+ if (!c->data_in_input_filters && !c->aborted) {
+ apr_bucket *e;
e = apr_bucket_flush_create(c->bucket_alloc);
/* We just send directly to the connection based filters. At
@@ -267,8 +341,12 @@ static void check_pipeline_flush(request_rec *r)
* of the request filters). We just want to flush the buckets
* if something hasn't been sent to the network yet.
*/
+ apr_brigade_cleanup(bb);
APR_BRIGADE_INSERT_HEAD(bb, e);
- ap_pass_brigade(r->connection->output_filters, bb);
+ ap_pass_brigade(c->output_filters, bb);
+ }
+
+ apr_brigade_destroy(bb);
}
void ap_process_request(request_rec *r)
