[PATCH] Allow Partial Content responses to satisfy Range requests

Maxim Dounin mdounin at mdounin.ru
Fri Dec 5 16:00:43 UTC 2014


Hello!

On Thu, Dec 04, 2014 at 09:07:57PM +0000, Steven Hartland wrote:

> # HG changeset patch
> # User Steven Hartland <steven.hartland at multiplay.co.uk>
> # Date 1417727204 0
> #      Thu Dec 04 21:06:44 2014 +0000
> # Node ID 05d3973ece9af030d0312932938fc3d1f2f139dd
> # Parent  1573fc7875fa09ee55763ce7ddc4e98d61e1deaf
> Allow Partial Content responses to satisfy Range requests

I would suggest something like:

Range filter: support for 206 Partial Content responses.

Some explanation on how to use these changes may be also helpful, 
as out of the box nginx won't be able to do anything good with it.

In general I think that this change is interesting, but there are 
some rough edges.  See below for various comments.

> 
> diff -r 1573fc7875fa -r 05d3973ece9a src/http/modules/ngx_http_range_filter_module.c
> --- a/src/http/modules/ngx_http_range_filter_module.c	Wed Nov 26 18:35:37 2014 -0800
> +++ b/src/http/modules/ngx_http_range_filter_module.c	Thu Dec 04 21:06:44 2014 +0000
> @@ -54,6 +54,7 @@
>  
>  typedef struct {
>      off_t        offset;
> +    off_t        content_length;
>      ngx_str_t    boundary_header;
>      ngx_array_t  ranges;
>  } ngx_http_range_filter_ctx_t;
> @@ -65,7 +66,8 @@
>      ngx_http_range_filter_ctx_t *ctx);
>  static ngx_int_t ngx_http_range_multipart_header(ngx_http_request_t *r,
>      ngx_http_range_filter_ctx_t *ctx);
> -static ngx_int_t ngx_http_range_not_satisfiable(ngx_http_request_t *r);
> +static ngx_int_t ngx_http_range_not_satisfiable(ngx_http_request_t *r,
> +    ngx_http_range_filter_ctx_t *ctx);
>  static ngx_int_t ngx_http_range_test_overlapped(ngx_http_request_t *r,
>      ngx_http_range_filter_ctx_t *ctx, ngx_chain_t *in);
>  static ngx_int_t ngx_http_range_singlepart_body(ngx_http_request_t *r,
> @@ -76,6 +78,9 @@
>  static ngx_int_t ngx_http_range_header_filter_init(ngx_conf_t *cf);
>  static ngx_int_t ngx_http_range_body_filter_init(ngx_conf_t *cf);
>  
> +static ngx_int_t ngx_http_content_range_parse(ngx_http_request_t *r,
> +    ngx_http_range_filter_ctx_t *ctx);
> +

Functions in the range filter use "ngx_http_range_*" prefix, so 
there should be a better name.

Also, both the declaration and function body should be placed 
eslewhere, see below.

>  
>  static ngx_http_module_t  ngx_http_range_header_filter_module_ctx = {
>      NULL,                                  /* preconfiguration */
> @@ -153,8 +158,8 @@
>      ngx_http_range_filter_ctx_t  *ctx;
>  
>      if (r->http_version < NGX_HTTP_VERSION_10
> -        || r->headers_out.status != NGX_HTTP_OK
> -        || r != r->main
> +        || (r->headers_out.status != NGX_HTTP_OK
> +        && r->headers_out.status != NGX_HTTP_PARTIAL_CONTENT)
>          || r->headers_out.content_length_n == -1
>          || !r->allow_ranges)
>      {

Style, should be:

@@ -153,8 +158,8 @@ ngx_http_range_header_filter(ngx_http_re
     ngx_http_range_filter_ctx_t  *ctx;
 
     if (r->http_version < NGX_HTTP_VERSION_10
-        || r->headers_out.status != NGX_HTTP_OK
-        || r != r->main
+        || (r->headers_out.status != NGX_HTTP_OK
+            && r->headers_out.status != NGX_HTTP_PARTIAL_CONTENT)
         || r->headers_out.content_length_n == -1
         || !r->allow_ranges)
     {

> @@ -230,26 +235,31 @@
>  
>      ranges = r->single_range ? 1 : clcf->max_ranges;
>  
> -    switch (ngx_http_range_parse(r, ctx, ranges)) {
> +    switch (ngx_http_content_range_parse(r, ctx)) {
> +    case NGX_OK:

If there are only two possible results of 
ngx_http_content_range_parse(), there is no real reason to use 
switch here.  Just an if with "goto next_filter" should do.

> +        switch (ngx_http_range_parse(r, ctx, ranges)) {
> +        case NGX_OK:
> +            ngx_http_set_ctx(r, ctx, ngx_http_range_body_filter_module);
>  
> -    case NGX_OK:
> -        ngx_http_set_ctx(r, ctx, ngx_http_range_body_filter_module);
> +            r->headers_out.status = NGX_HTTP_PARTIAL_CONTENT;
> +            r->headers_out.status_line.len = 0;
>  
> -        r->headers_out.status = NGX_HTTP_PARTIAL_CONTENT;
> -        r->headers_out.status_line.len = 0;
> +            if (ctx->ranges.nelts == 1) {
> +                return ngx_http_range_singlepart_header(r, ctx);
> +            }
>  
> -        if (ctx->ranges.nelts == 1) {
> -            return ngx_http_range_singlepart_header(r, ctx);
> +            return ngx_http_range_multipart_header(r, ctx);
> +
> +        case NGX_HTTP_RANGE_NOT_SATISFIABLE:
> +            return ngx_http_range_not_satisfiable(r, ctx);
> +
> +        case NGX_ERROR:
> +            return NGX_ERROR;
> +
> +        default: /* NGX_DECLINED */
> +            break;
>          }
> -
> -        return ngx_http_range_multipart_header(r, ctx);
> -
> -    case NGX_HTTP_RANGE_NOT_SATISFIABLE:
> -        return ngx_http_range_not_satisfiable(r);
> -
> -    case NGX_ERROR:
> -        return NGX_ERROR;
> -
> +        break;
>      default: /* NGX_DECLINED */
>          break;
>      }
> @@ -274,13 +284,12 @@

Please put

[diff]
showfunc=1

into your ~/.hgrc, it'll make patches more readable.  Thanks.

>      ngx_uint_t ranges)
>  {
>      u_char            *p;
> -    off_t              start, end, size, content_length;
> +    off_t              start, end, size;
>      ngx_uint_t         suffix;
>      ngx_http_range_t  *range;
>  
>      p = r->headers_in.range->value.data + 6;
>      size = 0;
> -    content_length = r->headers_out.content_length_n;

Just using ctx->content_length here may be a better option.

>  
>      for ( ;; ) {
>          start = 0;
> @@ -298,6 +307,10 @@
>                  start = start * 10 + *p++ - '0';
>              }
>  
> +            if (start < ctx->offset) {
> +                return NGX_HTTP_RANGE_NOT_SATISFIABLE;
> +            }
> +

That's wrong return code.

Additionally, this probably needs some logging, as this situation 
indicates a problem elsewhere: a 206 Partial Content response was 
used to answer a request for a range not covered by the response.

>              while (*p == ' ') { p++; }
>  
>              if (*p++ != '-') {
> @@ -307,7 +320,7 @@
>              while (*p == ' ') { p++; }
>  
>              if (*p == ',' || *p == '\0') {
> -                end = content_length;
> +                end = ctx->content_length;
>                  goto found;
>              }
>  
> @@ -331,12 +344,12 @@
>          }
>  
>          if (suffix) {
> -            start = content_length - end;
> -            end = content_length - 1;
> +            start = ctx->content_length - end;
> +            end = ctx->content_length - 1;
>          }
>  
> -        if (end >= content_length) {
> -            end = content_length;
> +        if (end >= ctx->content_length) {
> +            end = ctx->content_length;
>  
>          } else {
>              end++;
> @@ -369,7 +382,7 @@
>          return NGX_HTTP_RANGE_NOT_SATISFIABLE;
>      }
>  
> -    if (size > content_length) {
> +    if (size > ctx->content_length) {
>          return NGX_DECLINED;
>      }
>  
> @@ -384,16 +397,18 @@
>      ngx_table_elt_t   *content_range;
>      ngx_http_range_t  *range;
>  
> -    content_range = ngx_list_push(&r->headers_out.headers);
> -    if (content_range == NULL) {
> -        return NGX_ERROR;
> +    if (r->headers_out.content_range == NULL) {
> +        content_range = ngx_list_push(&r->headers_out.headers);
> +        if (content_range == NULL) {
> +            return NGX_ERROR;
> +        }
> +        r->headers_out.content_range = content_range;
> +        content_range->hash = 1;
> +        ngx_str_set(&content_range->key, "Content-Range");
> +    } else {
> +        content_range = r->headers_out.content_range;
>      }
>  
> -    r->headers_out.content_range = content_range;
> -
> -    content_range->hash = 1;
> -    ngx_str_set(&content_range->key, "Content-Range");
> -
>      content_range->value.data = ngx_pnalloc(r->pool,
>                                      sizeof("bytes -/") - 1 + 3 * NGX_OFF_T_LEN);
>      if (content_range->value.data == NULL) {

It might be easier to just remove an old header if it's here, like 
this:

@@ -389,6 +389,10 @@ ngx_http_range_singlepart_header(ngx_htt
         return NGX_ERROR;
     }
 
+    if (r->headers_out.content_range) {
+        r->headers_out.content_range->hash = 0;
+    }
+
     r->headers_out.content_range = content_range;
 
     content_range->hash = 1;

Additionally, it looks like appropriate modifications to 
ngx_http_range_multipart_header() are missing.  This will result 
in unneeded/confusing Content-Range header in multipart/ranges 
responses produced from 206 responses.

> @@ -407,7 +422,7 @@
>      content_range->value.len = ngx_sprintf(content_range->value.data,
>                                             "bytes %O-%O/%O",
>                                             range->start, range->end - 1,
> -                                           r->headers_out.content_length_n)
> +                                           ctx->content_length)
>                                 - content_range->value.data;
>  
>      r->headers_out.content_length_n = range->end - range->start;
> @@ -546,22 +561,25 @@
>  
>  
>  static ngx_int_t
> -ngx_http_range_not_satisfiable(ngx_http_request_t *r)
> +ngx_http_range_not_satisfiable(ngx_http_request_t *r,
> +    ngx_http_range_filter_ctx_t *ctx)
>  {
>      ngx_table_elt_t  *content_range;
>  
>      r->headers_out.status = NGX_HTTP_RANGE_NOT_SATISFIABLE;
>  
> -    content_range = ngx_list_push(&r->headers_out.headers);
> -    if (content_range == NULL) {
> -        return NGX_ERROR;
> +    if (r->headers_out.content_range == NULL) {
> +        content_range = ngx_list_push(&r->headers_out.headers);
> +        if (content_range == NULL) {
> +            return NGX_ERROR;
> +        }
> +        r->headers_out.content_range = content_range;
> +        content_range->hash = 1;
> +        ngx_str_set(&content_range->key, "Content-Range");
> +    } else {
> +        content_range = r->headers_out.content_range;
>      }
>  
> -    r->headers_out.content_range = content_range;
> -
> -    content_range->hash = 1;
> -    ngx_str_set(&content_range->key, "Content-Range");
> -
>      content_range->value.data = ngx_pnalloc(r->pool,
>                                         sizeof("bytes */") - 1 + NGX_OFF_T_LEN);
>      if (content_range->value.data == NULL) {

Same here.


> @@ -570,7 +588,7 @@
>  
>      content_range->value.len = ngx_sprintf(content_range->value.data,
>                                             "bytes */%O",
> -                                           r->headers_out.content_length_n)
> +                                           ctx->content_length)
>                                 - content_range->value.data;
>  
>      ngx_http_clear_content_length(r);
> @@ -888,3 +906,76 @@
>  
>      return NGX_OK;
>  }
> +
> +
> +static ngx_int_t
> +ngx_http_content_range_parse(ngx_http_request_t *r,
> +    ngx_http_range_filter_ctx_t *ctx)
> +{

This should be placed elsewhere.  Normally we functions in the 
order of execution.  That is, as this function is called from 
ngx_http_range_header_filter(), it should be placed after 
ngx_http_range_header_filter().  And as it's called just before 
ngx_http_range_parse(), it should be placed somewhere above 
ngx_http_range_parse().

> +    u_char     *p;
> +    off_t      start, end, len;

Style, should be:

    off_t    start, end, len;
    u_char  *p;

> +
> +    ctx->offset = 0;
> +    ctx->content_length = r->headers_out.content_length_n;
> +
> +    if (r->headers_out.status != NGX_HTTP_PARTIAL_CONTENT) {
> +       return NGX_OK;
> +    }

Style, should be 4 spaces.

> +
> +    if (r->headers_out.content_range == NULL
> +        || r->headers_out.content_range->value.len == 0) {

Style, "{" should be on it's own line.

> +       return NGX_HTTP_RANGE_NOT_SATISFIABLE;
> +    }

NGX_HTTP_RANGE_NOT_SATISFIABLE is wrong and confusing here.  It's 
not checked/used by the calling code, but nevertheless shouldn't 
be here.

The same applies to other uses of NGX_HTTP_RANGE_NOT_SATISFIABLE 
in this function.

> +
> +    if (r->headers_out.content_range->value.len < 7
> +        || ngx_strncasecmp(r->headers_out.content_range->value.data,
> +                           (u_char *) "bytes ", 6) != 0) {
> +       return NGX_DECLINED;
> +    }

Style, "{" should be on it's own line.

> +
> +    start = 0;
> +    end = 0;
> +    len = 0;
> +
> +    p = r->headers_out.content_range->value.data + 6;
> +
> +    while (*p == ' ') { p++; }
> +
> +    if (*p < '0' || *p > '9') {
> +        return NGX_HTTP_RANGE_NOT_SATISFIABLE;
> +    }
> +
> +    while (*p >= '0' && *p <= '9') {
> +        start = start * 10 + *p++ - '0';
> +    }
> +
> +    if (*p++ != '-') {
> +        return NGX_HTTP_RANGE_NOT_SATISFIABLE;
> +    }
> +
> +    while (*p >= '0' && *p <= '9') {
> +        end = end * 10 + *p++ - '0';
> +    }
> +
> +    if (*p++ != '/') {
> +        return NGX_HTTP_RANGE_NOT_SATISFIABLE;
> +    }
> +
> +    if (*p < '0' || *p > '9') {
> +        return NGX_HTTP_RANGE_NOT_SATISFIABLE;
> +    }
> +
> +    while (*p >= '0' && *p <= '9') {
> +        len = len * 10 + *p++ - '0';
> +    }
> +
> +    if (*p != '\0') {
> +        return NGX_HTTP_RANGE_NOT_SATISFIABLE;
> +    }
> +
> +    ctx->offset = start;
> +    ctx->content_length = len;
> +
> +    return NGX_OK;
> +}
> +
> diff -r 1573fc7875fa -r 05d3973ece9a src/http/ngx_http_upstream.c
> --- a/src/http/ngx_http_upstream.c	Wed Nov 26 18:35:37 2014 -0800
> +++ b/src/http/ngx_http_upstream.c	Thu Dec 04 21:06:44 2014 +0000
> @@ -292,6 +292,11 @@
>                   ngx_http_upstream_copy_content_encoding, 0, 0 },
>  #endif
>  
> +    { ngx_string("Content-Range"),
> +                 ngx_http_upstream_ignore_header_line, 0,
> +                 ngx_http_upstream_copy_allow_ranges,
> +                 offsetof(ngx_http_headers_out_t, content_range), 1 },
> +

Placing this right after Accept-Ranges may be better.

>      { ngx_null_string, NULL, 0, NULL, 0, 0 }
>  };
>  
> @@ -4516,37 +4521,26 @@
>  ngx_http_upstream_copy_allow_ranges(ngx_http_request_t *r,
>      ngx_table_elt_t *h, ngx_uint_t offset)
>  {
> -    ngx_table_elt_t  *ho;
> -
>      if (r->upstream->conf->force_ranges) {
>          return NGX_OK;
>      }
> -
>  #if (NGX_HTTP_CACHE)
> -

Style, empty lines should be here.

>      if (r->cached) {
>          r->allow_ranges = 1;
> -        return NGX_OK;
> +        if (offsetof(ngx_http_headers_out_t, accept_ranges) == offset) {
> +            return NGX_OK;
> +       }
>      }

It may be better idea to introduce a separate copy function 
instead.

>  
>      if (r->upstream->cacheable) {
>          r->allow_ranges = 1;
>          r->single_range = 1;
> -        return NGX_OK;
> -    }
> -
> +        if (offsetof(ngx_http_headers_out_t, accept_ranges) == offset) {
> +            return NGX_OK;
> +        }
> +    }
>  #endif
> -
> -    ho = ngx_list_push(&r->headers_out.headers);
> -    if (ho == NULL) {
> -        return NGX_ERROR;
> -    }
> -
> -    *ho = *h;
> -
> -    r->headers_out.accept_ranges = ho;
> -
> -    return NGX_OK;
> +    return ngx_http_upstream_copy_header_line(r, h, offset);

I don't think that ngx_http_upstream_copy_header_line() worth it.

>  }
>  
>  
> 
> _______________________________________________
> nginx-devel mailing list
> nginx-devel at nginx.org
> http://mailman.nginx.org/mailman/listinfo/nginx-devel

-- 
Maxim Dounin
http://nginx.org/



More information about the nginx-devel mailing list