httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Luca Toscano <toscano.l...@gmail.com>
Subject Re: svn commit: r1754732 - /httpd/httpd/trunk/modules/proxy/mod_proxy_fcgi.c
Date Tue, 02 Aug 2016 06:22:21 GMT
Hi Yann,

thanks a lot for the review, answer inline:

2016-08-02 1:03 GMT+02:00 Yann Ylavic <ylavic.dev@gmail.com>:

> On Mon, Aug 1, 2016 at 12:55 PM,  <elukey@apache.org> wrote:
> >
> > Modified: httpd/httpd/trunk/modules/proxy/mod_proxy_fcgi.c
> > URL:
> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/proxy/mod_proxy_fcgi.c?rev=1754732&r1=1754731&r2=1754732&view=diff
> >
> ==============================================================================
> > --- httpd/httpd/trunk/modules/proxy/mod_proxy_fcgi.c (original)
> > +++ httpd/httpd/trunk/modules/proxy/mod_proxy_fcgi.c Mon Aug  1 10:55:03
> 2016
> > @@ -661,14 +661,26 @@ recv_again:
> >                                      break;
> >                                  }
> >                                  else if (status == HTTP_NOT_MODIFIED) {
> > -                                    /* The 304 response MUST NOT contain
> > -                                     * a message-body, ignore it.
> > -                                     * The break is not added since
> there might
> > -                                     * be more bytes to read from the
> FCGI
> > -                                     * connection. Even if the
> message-body is
> > -                                     * ignored we want to avoid
> subsequent
> > -                                     * bogus reads. */
> > +                                    /* A 304 response MUST NOT contain
> > +                                     * a message-body, so we must
> ignore it but
> > +                                     * some extra steps needs to be
> taken to
> > +                                     * avoid inconsistencies.
> > +                                     * The break is not added with
> connection
> > +                                     * reuse set since there might be
> more bytes
> > +                                     * to read from the FCGI connection,
> > +                                     * like the message-body,
>
> There is no more to read for *this* request, it has its response: 304
> (header only).

If we read something that's for the next request (some response for no
> request), so we can discard it (conn->close=1 and done+break).
> But that'd better be checked just before reusing the connection
> (before sending the next request), the later detected the better; if
> anything read do not reuse (close) and establish a new one.


> > +                                     * subsequent bogus reads (for
> example
> > +                                     * the start of the message-body
> > +                                     * interpreted as FCGI header).
> > +                                     * With connecton reuse disabled
> (default)
> > +                                     * we can safely break and force
> the end
> > +                                     * of the FCGI processing phase
> since the
> > +                                     * connection will be cleaned up
> later on. */
> >                                      ignore_body = 1;
> > +                                    if (conn->close) {
> > +                                        done = 1;
> > +                                        break;
> > +                                    }
>
> So this does not look correct to me, disablereuse or "connection:
> close" shouldn't control the behaviour, unless we close+break below by
> reading data with ignore_body == 1 (but that's too early IHMO).
>
>
So IIUC you are saying to always done+break in the 304 use case (to avoid
reading from the connection again), and then detect the response in another
place. Would you mind to give me some indication about where this check
should be? I am reviewing the code but it is not straightforward to me
where to make this change.

Regards,

Luca

Mime
View raw message