PR 31759 - default handler returns output filter apr_status_t value

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

Some discussion has taken place in bugzilla but more discussion and
opinions are needed.

A patch recently posted:

Index: server/core.c
===================================================================
--- server/core.c       (revision 386843)
+++ server/core.c       (working copy)
@@ -3645,7 +3645,16 @@
         e = apr_bucket_eos_create(c->bucket_alloc);
         APR_BRIGADE_INSERT_TAIL(bb, e);

-        return ap_pass_brigade(r->output_filters, bb);
+        status = ap_pass_brigade(r->output_filters, bb);
+        if (status == APR_SUCCESS
+            || r->status != HTTP_OK
+            || c->aborted) {
+            return r->status;
+        }
+        else {
+            /* no way to know what type of error occurred */
+            return HTTP_INTERNAL_SERVER_ERROR;
+        }
     }
     else {              /* unusual method (not GET or POST) */
         if (r->method_number == M_INVALID) {

Some concerns about the else path:

a) is 500 proper?  should it just return OK instead?

b) what about logging that path to ensure that the administrator has
some help diagnosing the problem, since we can't carry the
apr_status_t any further than here?

There are concerns of too much logging (filter has already logged
something) or too little logging (filter didn't log anything).  The
access log won't have any strong hints that something bad happened
since r->status didn't get changed and c->aborted didn't get set.

Thoughts?

Re: PR 31759 - default handler returns output filter apr_status_t value

[Ruediger Pluem <rp...@apache.org>]

On 04/02/2006 12:27 AM, Jeff Trawick wrote:

> 
> yep ;)
> 
> I'll move the patch to trunk, change it to return OK where original
> patch returned r->status, integrate your logging patch, and commit, if
> it holds up under more varied testing.
> 

Thanks.

Re: PR 31759 - default handler returns output filter apr_status_t value

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

On 4/1/06, Ruediger Pluem <rp...@apache.org> wrote:
>
>
> On 03/31/2006 06:53 PM, Jeff Trawick wrote:
>
> >
> > Some concerns about the else path:
> >
> > a) is 500 proper?  should it just return OK instead?
>
> I think 500 is good.
>
> >
> > b) what about logging that path to ensure that the administrator has
> > some help diagnosing the problem, since we can't carry the
> > apr_status_t any further than here?
> >
> > There are concerns of too much logging (filter has already logged
> > something) or too little logging (filter didn't log anything).  The
> > access log won't have any strong hints that something bad happened
> > since r->status didn't get changed and c->aborted didn't get set.
>
> What about the debug level for this log message? Would this be a compromise
> between too much logging and no logging?

yep ;)

I'll move the patch to trunk, change it to return OK where original
patch returned r->status, integrate your logging patch, and commit, if
it holds up under more varied testing.

Re: PR 31759 - default handler returns output filter apr_status_t value

[Ruediger Pluem <rp...@apache.org>]

On 03/31/2006 06:53 PM, Jeff Trawick wrote:

> 
> Some concerns about the else path:
> 
> a) is 500 proper?  should it just return OK instead?

I think 500 is good.

> 
> b) what about logging that path to ensure that the administrator has
> some help diagnosing the problem, since we can't carry the
> apr_status_t any further than here?
> 
> There are concerns of too much logging (filter has already logged
> something) or too little logging (filter didn't log anything).  The
> access log won't have any strong hints that something bad happened
> since r->status didn't get changed and c->aborted didn't get set.

What about the debug level for this log message? Would this be a compromise
between too much logging and no logging?

Regards

Rüdiger