Re: svn commit: r1681199 - /httpd/httpd/branches/2.4.x/STATUS

[Yann Ylavic <yl...@gmail.com>]

Hi Jeff,

On Fri, May 22, 2015 at 9:21 PM,  <tr...@apache.org> wrote:
>
> +     trawick: It still looks to me that an error with ap_pass_brigade (towards
> +              client) can turn into a 400 error, which is what I was concerned
> +              about originally.

I don't see where this can happen, but I may be missing something,
please correct me if I'm wrong.

Whenever ap_pass_brigade() is called, failing or not, the data_sent
flag is set to 1 (some data have been sent to the client).
Now outside the loop, in both cases where {client,backend}_failed
(i.e. any read or write error occured on the {client,backend} side) or
!request_ended (i.e. the last chunk was not received from the backend,
yet another misnaming), the final rv will be set to DONE (and won't
change until return, an error bucket 503 may be used if backend_failed
but not if the client connection was aborted).

I agree that the state maintained by as much variables is a mess (the
code has probably evolved by adding a new one to handle a new case, I
did not add any in this patch though), and we ought to simplify this,
but maybe we can leave it as is for 2.4.13 (and fix PR 56823) and
refactor soon?
I'd volonteer eventually...

Regards,
Yann.

Re: svn commit: r1681199 - /httpd/httpd/branches/2.4.x/STATUS

[Jeff Trawick <tr...@gmail.com>]

On Fri, May 22, 2015 at 4:58 PM, Yann Ylavic <yl...@gmail.com> wrote:

> Hi Jeff,
>
> On Fri, May 22, 2015 at 9:21 PM,  <tr...@apache.org> wrote:
> >
> > +     trawick: It still looks to me that an error with ap_pass_brigade
> (towards
> > +              client) can turn into a 400 error, which is what I was
> concerned
> > +              about originally.
>
> I don't see where this can happen, but I may be missing something,
> please correct me if I'm wrong.
>
> Whenever ap_pass_brigade() is called, failing or not, the data_sent
> flag is set to 1 (some data have been sent to the client).
> Now outside the loop, in both cases where {client,backend}_failed
> (i.e. any read or write error occured on the {client,backend} side) or
> !request_ended (i.e. the last chunk was not received from the backend,
> yet another misnaming), the final rv will be set to DONE (and won't
> change until return, an error bucket 503 may be used if backend_failed
> but not if the client connection was aborted).
>

I see for fleeting moments that there's a sequence of setting of different
variables to certain values over the function that ensures that
HTTP_BAD_REQUEST is not set if an error occurred writing to the client.
Fun.

I'm sorry for the trouble.


> I agree that the state maintained by as much variables is a mess (the
> code has probably evolved by adding a new one to handle a new case, I
> did not add any in this patch though), and we ought to simplify this,
> but maybe we can leave it as is for 2.4.13 (and fix PR 56823) and
> refactor soon?
> I'd volonteer eventually...
>
> Regards,
> Yann.
>



-- 
Born in Roswell... married an alien...
http://emptyhammock.com/