[PATCH] Make ngx_http_upstream provide a way to expose errors after sending out the response header

[agentzh]

Hello!

On Mon, Sep 10, 2012 at 3:35 AM, Maxim Dounin <mdounin at mdounin.ru> wrote:
>
> I don't like the idea of changing r->headers_out.status here.
> It's really incorrect and confusing: r->headers_out.status is a
> status code of a response, and it's way too late to change it when
> we've found an error during sending the response body.
>

I don't like it either. Just cannot think of an existing neutral flag
that can be made use of.

>> --- nginx-1.2.3/src/http/ngx_http_upstream.c  2012-08-06 10:34:08.000000000 -0700
>> +++ nginx-1.2.3-patched/src/http/ngx_http_upstream.c  2012-09-09
>> 21:58:04.727761891 -0700
>> @@ -2383,7 +2383,7 @@
>
> Just a side note: please use -p when submitting patches.
>

Okay, I will at the next time :)

>> @@ -2430,13 +2430,17 @@
>>              if (u->busy_bufs == NULL) {
>>
>>                  if (u->length == 0
>> -                    || upstream->read->eof
>> -                    || upstream->read->error)
>> +                    || (upstream->read->eof &&
>> u->headers_in.content_length_n == -1))
>
> Note: you can't be sure that eof with u->headers_in.content_length_n ==
> -1 is ok.  E.g. with chunked encoding used it's certainly not
> unless u->length == 0 as well.
>

Yes, I was ware of this hole. Just could not think of a solution for
this. Any suggestion?

> While content_length_n check is already used in cache and store
> cases in ngx_http_upstream_process_request(), we might want to get
> rid of it instead of reintroducing it in various other places.
>

Can I ask how will you get rid of this? Any plans in your head?

>>                  {
>>                      ngx_http_upstream_finalize_request(r, u, 0);
>>                      return;
>>                  }
>>
>> +                if (upstream->read->eof || upstream->read->error) {
>> +                    ngx_http_upstream_finalize_request(r, u,
>> NGX_HTTP_BAD_GATEWAY);
>> +                    return;
>> +                }
>> +
>
> Just a side note: resulting code looks not very readable for me.
>

Yes, I could have done a little better here :)

>
> Additionally, I think major problem here isn't
> subrequest-in-memory special case, but sending buf with "last"
> set, which in some situations may result in truncated response
> being considered full by a downstream.
>

But unfortunately, in the context of subrequests, the "last_buf" flag
is already never set anyway. So we cannot use this flag as an
indicator for data truncation errors anyway.

Best regards,
-agentzh