[patch] handle write errors more gracefully

[James Brown]

If a client goes away while mod_wsgi is writing (causing ap_pass_brigate to fail), it currently raises an IOError, which causes an entry in Apache's error log. There is some code that tries to detect disconnects and optionally log a debug statement instead, but it doesn't work very well.

Additionally, if the client goes away during a write, the ap_bucket_brigade is never cleaned up.

I believe that the attached patch will resolve both issues.

[Graham Dumpleton]

I will look at it, but I recollect both things are as they are for a reason.

First off, the bucket brigade will be cleaned up in as much no memory
will leak because of how Apache memory pooling works.

Doing the cleanup when an error occurs is possibly not meant to be
done per rules for using Apache bucket API because it technically is
in an unknown state at that point. Trying to do a cleanup could
therefore result in undefined behaviour.

As for whether messages, when yielding response from an application,
either way a message is going to end up in the logs and nothing more.
Your issue therefore seems to be the log level of messages in logs and
your wanting to hide them as much as possible.

My recollection of why it is done the way it is is that an error at
that point can actually indicate a problem in the Apache output filter
chain and doesn't just mean that an error occurred in sending data to
the client. Unfortunately mod_ssl seems to indicate errors when issue
was a closed client connection because SSL will flag an error if in
middle of some SSL exchange.

In other words, one should necessarily be masking what could be a true
actual error or problem rather than just noise. I would really need to
try and find old emails about this as has been discussed before.

Graham

[James Brown]

Responses inline:

On Thu, Apr 19, 2012 at 7:59 PM, Graham Dumpleton <graham.d...@gmail.com> wrote:

First off, the bucket brigade will be cleaned up in as much no memory
will leak because of how Apache memory pooling works.

I mostly changed it because that is how I've seen other apache modules do it. I'll believe you if you say that the brigade won't leak. I hate reading APR documentation

As for whether messages, when yielding response from an application,
either way a message is going to end up in the logs and nothing more.
Your issue therefore seems to be the log level of messages in logs and
your wanting to hide them as much as possible.

My recollection of why it is done the way it is is that an error at
that point can actually indicate a problem in the Apache output filter
chain and doesn't just mean that an error occurred in sending data to
the client. Unfortunately mod_ssl seems to indicate errors when issue
was a closed client connection because SSL will flag an error if in
middle of some SSL exchange.

In other words, one should necessarily be masking what could be a true
actual error or problem rather than just noise. I would really need to
try and find old emails about this as has been discussed before.

It could be an actual problem. But, in practice over an enormous number of requests, it seems to happen almost any time a client disconnects mid-write (and, no, we're not using mod_ssl), and having all of these errors in our log was actually masking more actual problems than hiding them would. If nothing else, it would be nice to make there be a configuration flag to avoid printing enormous amounts of this spam for configurations where we know there's nothing interesting farther down the filter chain.

 -- 

James Brown

Systems Engineer

Yelp, Inc.

[Graham Dumpleton]

On 20 April 2012 13:52, James Brown <jbr...@yelp.com> wrote:
> Responses inline:
>
> On Thu, Apr 19, 2012 at 7:59 PM, Graham Dumpleton
> <graham.d...@gmail.com> wrote:
>>
>> First off, the bucket brigade will be cleaned up in as much no memory
>> will leak because of how Apache memory pooling works.
>
>
> I mostly changed it because that is how I've seen other apache modules do
> it. I'll believe you if you say that the brigade won't leak. I hate reading
> APR documentation

It looks to be one of those things which is harmless to call if an error.

APU_DECLARE(apr_status_t) apr_brigade_cleanup(void *data)
{
apr_bucket_brigade *b = data;
apr_bucket *e;

while (!APR_BRIGADE_EMPTY(b)) {
e = APR_BRIGADE_FIRST(b);
apr_bucket_delete(e);
}
/* We don't need to free(bb) because it's allocated from a pool. */
return APR_SUCCESS;
}

APU_DECLARE(apr_status_t) apr_brigade_destroy(apr_bucket_brigade *b)
{
apr_pool_cleanup_kill(b->p, b, brigade_cleanup);
return apr_brigade_cleanup(b);
}

At the moment it would be getting picked up when the pool calls
apr_brigade_destroy() on the pool being destroyed.

>> As for whether messages, when yielding response from an application,
>> either way a message is going to end up in the logs and nothing more.
>> Your issue therefore seems to be the log level of messages in logs and
>> your wanting to hide them as much as possible.
>>
>> My recollection of why it is done the way it is is that an error at
>> that point can actually indicate a problem in the Apache output filter
>> chain and doesn't just mean that an error occurred in sending data to
>> the client. Unfortunately mod_ssl seems to indicate errors when issue
>> was a closed client connection because SSL will flag an error if in
>> middle of some SSL exchange.
>>
>> In other words, one should necessarily be masking what could be a true
>> actual error or problem rather than just noise. I would really need to
>> try and find old emails about this as has been discussed before.
>
> It could be an actual problem. But, in practice over an enormous number of
> requests, it seems to happen almost any time a client disconnects mid-write
> (and, no, we're not using mod_ssl), and having all of these errors in our
> log was actually masking more actual problems than hiding them would. If
> nothing else, it would be nice to make there be a configuration flag to
> avoid printing enormous amounts of this spam for configurations where we
> know there's nothing interesting farther down the filter chain.

I will have a bit more of a look at it and think about. Timing is
certainly good as just last weekend did first bit of serious work on
mod_wsgi for a long time, even if was only back porting stuff from 4.0
development version so could release a mod_wsgi 3.4 some time soon.

Graham