First off thanks for reviewing, comments / questions inline below

On 05/12/2014 16:00, Maxim Dounin wrote:
> Hello!
>
> On Thu, Dec 04, 2014 at 09:07:57PM +0000, Steven Hartland wrote:
>
>> # HG changeset patch
>> # User Steven Hartland <steven.hartland@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.
Looks like I lost the full comment when trying to make hg do a squash,
its not as nice to work with as git, which I guess is why Google are
migrating their golang repo to github ;-)

I'll restore this in the next patch but for now here's some details.

The driver for the changes is an additional module we've been developing
which based on this core enhancement can use predictably split range
requests to satisfy other requests.

This allows nginx to cache the responses to client downloads that use
random ranges to achieve full file downloads, such clients include EA's
Origin, Microsoft's XBox and Blizzard's downloaders which source from
CDN's such as Akamai and Limelight Networks.

What the module does is rewrite the random range request sent by the
client into one or more ranges with predictable boundaries. Then in the
uncached case it makes the required requests to the origin allowing
nginx core to cache the results. As the responses come in it combines
them to satisfy the initial range request sent by the client. Of course
some may be previously cached responses.

The result is a caching proxy that can build up a full copy of the file
by downloading individual ranges, none of which overlap; without which
you see massive duplication in the cache, resulting not only in wasted
local space but also slower downloads as the cache does multiple
requests for the same data from the origin server.

This could also be used to return a quicker response to the client when
caching a large file from an upstream source. Currently in a multi
client scenario where you want to avoid duplicate requests its requires
the entire file to be downloaded and cached before the responses start.
If the file where split into ranges the clients would only have to wait
until the current chunk processed until it could start getting data,
reducing the risk for timeouts with large files.

More information on this can be seen on our blog here:
http://blog.multiplay.co.uk/2014/04/lancache-dynamically-caching-game-installs-at-lans-using-nginx/

> In general I think that this change is interesting, but there are
> some rough edges. See below for various comments.
>
>> 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.
Sorry I'm not sure I follow you correctly, do you mean set the old
content_length local from ctx->content_lenght keeping the diff minimal?
> @@ -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;
> }
> ...
>
> 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.
Nice catch will need to work on this one.
>> + 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.
Will look into these
>> { 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.
I removed as it was inconsistent with the function above, could you
confirm the details of this style requirement please so I know for the
future?
>
>> 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.

I thought it would be wasteful to copy the function instead of making
the small amendments to make it more generic and reuse
ngx_http_upstream_copy_header_line instead of a hand rolled version of it.

Its easy enough to make it independent if you think that's the better
option?

Regards
Steve

_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel