You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by William A Rowe Jr <wr...@rowe-clan.net> on 2016/09/01 00:51:53 UTC

Re: svn commit: r1758672 - in /httpd/httpd/branches/2.2.x: ./ CHANGES include/httpd.h server/protocol.c

On Wed, Aug 31, 2016 at 2:52 PM, <yl...@apache.org> wrote:

> Author: ylavic
> Date: Wed Aug 31 19:52:28 2016
> New Revision: 1758672
>
> URL: http://svn.apache.org/viewvc?rev=1758672&view=rev
> Log:
> Merge r1710095, r1727544 from trunk:
>
> core: Limit to ten the number of tolerated empty lines between request,
>

Yup



> and consume them before the pipelining check to avoid possible response
> delay when reading the next request without flushing.
>

I don't think you backported this aspect. It was based on tweaking a lower
level lookahead that didn't exist in 2.2.x, unless I missed some backport
approvals?

Re: svn commit: r1758672 - in /httpd/httpd/branches/2.2.x: ./ CHANGES include/httpd.h server/protocol.c

Posted by Yann Ylavic <yl...@gmail.com>.
On Thu, Sep 1, 2016 at 10:14 AM, Yann Ylavic <yl...@gmail.com> wrote:
> On Thu, Sep 1, 2016 at 9:57 AM, Yann Ylavic <yl...@gmail.com> 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.

Re: svn commit: r1758672 - in /httpd/httpd/branches/2.2.x: ./ CHANGES include/httpd.h server/protocol.c

Posted by Yann Ylavic <yl...@gmail.com>.
On Thu, Sep 1, 2016 at 9:57 AM, Yann Ylavic <yl...@gmail.com> 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?

Re: svn commit: r1758672 - in /httpd/httpd/branches/2.2.x: ./ CHANGES include/httpd.h server/protocol.c

Posted by Yann Ylavic <yl...@gmail.com>.
On Thu, Sep 1, 2016 at 2:51 AM, William A Rowe Jr <wr...@rowe-clan.net> wrote:
> On Wed, Aug 31, 2016 at 2:52 PM, <yl...@apache.org> wrote:
>>
>> Author: ylavic
>> Date: Wed Aug 31 19:52:28 2016
>> New Revision: 1758672
>>
>> URL: http://svn.apache.org/viewvc?rev=1758672&view=rev
>> Log:
>> Merge r1710095, r1727544 from trunk:
>>
>> core: Limit to ten the number of tolerated empty lines between request,
>
>
> Yup
>
>
>>
>> and consume them before the pipelining check to avoid possible response
>> delay when reading the next request without flushing.
>
>
> I don't think you backported this aspect. It was based on tweaking a lower
> level lookahead that didn't exist in 2.2.x, unless I missed some backport
> approvals?

Right, this is done in check_pipeline() in 2.4.x, but wasn't part of
the proposed backport patch [1] (which I simply applied).

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.

[1] https://raw.githubusercontent.com/wrowe/patches/master/backport-2.2.x-r1710095-r1727544.patch