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

[Maxim Dounin]

Hello!

On Sun, Sep 09, 2012 at 10:03:24PM -0700, agentzh wrote:

> Hello!
> 
> According to the current implementation of ngx_http_upstream, there is
> almost no way for 3rd-party output body filters and "post_subrequest"
> handlers (in the subrequest context) to know if there's any errors
> while ngx_http_upstream is processing the upstream response body after
> the response header is sent out (in both the buffered and non-buffered
> modes).
> 
> For example, if
> 
> 1. a read-timeout happens in the middle of the process of reading the
> upstream response body,
> 2. or the upstream connection is closed prematurely in the same situation,
> 
> then ngx_http_upstream will just happily finalize the current request
> with the status code 0 (i.e., NGX_OK). This issue already affects at
> least our ngx_srcache and ngx_lua modules (originally reported by
> Bryan Alger).
> 
> Here attaches a patch that makes ngx_http_upstream set
> r->headers_out.status to a new error status code to notify the outside
> world if there is a problem. Comments will be highly appreciated as
> always :)

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.

> 
> Thanks!
> -agentzh
> 
> --- 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.

> 
>      if (c->read->timedout) {
>          ngx_connection_error(c, NGX_ETIMEDOUT, "upstream timed out");
> -        ngx_http_upstream_finalize_request(r, u, 0);
> +        ngx_http_upstream_finalize_request(r, u, NGX_HTTP_GATEWAY_TIME_OUT);
>          return;
>      }
> 
> @@ -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.

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.

>                  {
>                      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.

>                  b->pos = b->start;
>                  b->last = b->start;
>              }
> @@ -2710,7 +2714,16 @@
>  #if 0
>              ngx_http_busy_unlock(u->conf->busy_lock, &u->busy_lock);
>  #endif
> -            ngx_http_upstream_finalize_request(r, u, 0);
> +
> +            if (p->upstream_done
> +                || (p->upstream_eof && u->headers_in.content_length_n == -1))
> +            {
> +                ngx_http_upstream_finalize_request(r, u, 0);
> +
> +            } else {
> +                ngx_http_upstream_finalize_request(r, u, NGX_HTTP_BAD_GATEWAY);
> +            }
> +

Same here (well, two above comments about readability and 
content_length_n apply).

>              return;
>          }
>      }
> @@ -3073,6 +3086,13 @@
>          && rc != NGX_HTTP_REQUEST_TIME_OUT
>          && (rc == NGX_ERROR || rc >= NGX_HTTP_SPECIAL_RESPONSE))
>      {
> +        if (rc == NGX_ERROR) {
> +            r->headers_out.status = NGX_HTTP_INTERNAL_SERVER_ERROR;
> +
> +        } else {
> +            r->headers_out.status = rc;
> +        }
> +
>          rc = 0;
>      }

See above.

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.

Maxim Dounin