Error handling from filter modules

[Steven Hartland]

On 19/10/2015 17:59, Maxim Dounin wrote:
> Hello!
>
> On Fri, Oct 16, 2015 at 06:15:30PM +0100, Steven Hartland wrote:
>
>> On 16/10/2015 13:20, Maxim Dounin wrote:
>>> Hello!
>>>
>>> On Fri, Oct 16, 2015 at 02:36:13AM +0100, Steven Hartland wrote:
>>>
>>>> I'm making changes to a filter module and when it detected an error it
>>>> returned NGX_ERROR however the response generated to the client isn't
>>>> the expected 500 internal server error I would have expected given said
>>>> return.
>>>>
>>>> So the question is do filters have to manually call
>>>> ngx_http_finalize_request(r, NGX_HTTP_INTERNAL_SERVER_ERROR); or is it
>>>> expected that the upper layers should actually do the right thing and
>>>> ensure the client doesn't get a bad response generated from the current
>>>> state of r with no indication an error occurred?
>>> In filters, it's already to late to return anything in case of
>>> errors.  In body filters, it's way too late - the response was
>>> already partially sent.  And in header filters there is a chance that
>>> other filters allocated something response-specific, and an
>>> attempt to return a different response will break things.  So,
>>> when you return NGX_ERROR from a filter, the connection is just
>>> closed.
>> There are a few core filters which seem to contradict this;
>>
>> range does:
>> return ngx_http_range_not_satisfiable(r, ctx);
>>
>> while image does:
>> return ngx_http_filter_finalize_request(r,
>> &ngx_http_image_filter_module,
>> NGX_HTTP_UNSUPPORTED_MEDIA_TYPE);
> There is no contradiction here.  These are special cases which use
> ngx_http_filter_finalize_request(), as outlined later in the same
> message.
>
>> The range filter is the one I was looking at, picking back up the work you
>> gave feedback on a while back with regards allow partial content responses
>> to be used to satisfy range requests.
>>
>> During testing for failures I triggered the NGX_ERROR case here:
>> https://github.com/nginx/nginx/blob/master/src/http/modules/ngx_http_range_filter_module.c#L233
>>
>> When I triggered this case client recieved an broken 200 response, where as
>> if I did finalise the request I could deliver a 500 response, so was
>> wondering if this would be correct in this situation?
> When the NGX_ERROR case is triggered, no response is returned, the
> connection is just closed.
It does seem like the connection to the client is just closed however a 
206 response is logged for the request in the access log, hence my 
confusion; is that expected?
> As this expected to happen if and only
> if we weren't able to allocate memory, this is believed to be
> perfectly correct behaviour.  Trying to do anything else is very
> unlikely to succeed, but likely to result in additional problems.
>
In the current code yes, but in the patch that I was testing there's one 
additional case which can return an error. This was caused by an 
intentional configuration error (test designed to confirm the behaviour 
in this case).

Its likely going to be difficult to discuss fully without the patch to 
comment against so I'll finalise that and get it posted as soon as I can.

Thanks for all the feedback so far :)

     Regards
     Steve