Re: svn commit: r760167 - in /httpd/httpd/trunk: CHANGES server/util_script.c

["Roy T. Fielding" <ro...@gmail.com>]

-1 (veto)

Filling obscure areas of the server with stupid hacks that modify
the request structure because of something the content generator
*might* do is harmful to overall stability.  204 and 304 are already
handled elsewhere (or, if not, they should be handled elsewhere).

....Roy

On Mar 30, 2009, at 1:49 PM, niq@apache.org wrote:

> Author: niq
> Date: Mon Mar 30 20:49:10 2009
> New Revision: 760167
>
> URL: http://svn.apache.org/viewvc?rev=760167&view=rev
> Log:
> HTTPD script support
> When a script returns a no-body response code, pretend it was a HEAD
> request so as to drop any body the script might erroneously generate.
>
> Modified:
>     httpd/httpd/trunk/CHANGES
>     httpd/httpd/trunk/server/util_script.c
>
> Modified: httpd/httpd/trunk/CHANGES
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/CHANGES? 
> rev=760167&r1=760166&r2=760167&view=diff
> ====================================================================== 
> ========
> --- httpd/httpd/trunk/CHANGES [utf-8] (original)
> +++ httpd/httpd/trunk/CHANGES [utf-8] Mon Mar 30 20:49:10 2009
> @@ -2,6 +2,9 @@
>
>  Changes with Apache 2.3.3
>
> +  *) script support: avoid possibly sending contents with a 204 or  
> 304
> +     response.  PR 40953 [Nick Kew]
> +
>    *) ab: Fix a 100% CPU loop on platforms where a failed non- 
> blocking connect
>       returns EINPROGRESS and a subsequent poll() returns only  
> POLLERR.
>       Observed on HP-UX.  [Eric Covener]
>
> Modified: httpd/httpd/trunk/server/util_script.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/server/ 
> util_script.c?rev=760167&r1=760166&r2=760167&view=diff
> ====================================================================== 
> ========
> --- httpd/httpd/trunk/server/util_script.c (original)
> +++ httpd/httpd/trunk/server/util_script.c Mon Mar 30 20:49:10 2009
> @@ -462,6 +462,10 @@
>              if ((cgi_status == HTTP_UNSET) && (r->method_number ==  
> M_GET)) {
>                  cond_status = ap_meets_conditions(r);
>              }
> +            else if ((cgi_status == HTTP_NO_CONTENT) ||
> +                     (cgi_status == HTTP_NOT_MODIFIED)) {
> +                r->header_only = 1; /* discard any body */
> +            }
>              apr_table_overlap(r->err_headers_out, merge,
>                  APR_OVERLAP_TABLES_MERGE);
>              if (!apr_is_empty_table(cookie_table)) {
>
>

Re: svn commit: r760167 - in /httpd/httpd/trunk: CHANGES server/util_script.c

["Edward Z. Yang" <ez...@MIT.EDU>]

On Mon, 30 Mar 2009, Roy T. Fielding wrote:
> Filling obscure areas of the server with stupid hacks that modify
> the request structure because of something the content generator
> *might* do is harmful to overall stability.

A common error in content generators is to have gzip compression enabled
globally, and forget to turn it off for empty content responses
(resulting in some spurious gzip garbage being passed off).

> 204 and 304 are already
> handled elsewhere (or, if not, they should be handled elsewhere).

I did a quick grep through Apache's source code (for both the status
code and the #define constant), and the only relevant code I could find
was related to determining keep alive. Where would you recommend putting
the patch?

Cheers,
Edward

Re: svn commit: r760167 - in /httpd/httpd/trunk: CHANGES server/util_script.c

[Nick Kew <ni...@webthing.com>]

On 30 Mar 2009, at 23:48, Roy T. Fielding wrote:

> -1 (veto)

Reverted.

>
> Filling obscure areas of the server with stupid hacks that modify
> the request structure because of something the content generator
> *might* do is harmful to overall stability.

Up to a point, Lord Copper.  There are a lot of little hacks in the
request_rec to which your point could reasonably apply.
Perhaps this one would look better if the field were called
"no_body" rather than "header_only".

Given that startingpoint, which is better/worse:
   - tag a small fix onto a hack that serves the same purpose?
   - introduce a whole new fix that serves the same purpose?

>   204 and 304 are already
> handled elsewhere (or, if not, they should be handled elsewhere).

That's precisely what PR 40953 is about.

-- 
Nick Kew