From 3a9953e25c6c7c7f4a0090d139577815104bad9a Mon Sep 17 00:00:00 2001 From: Otto van der Schaaf Date: Fri, 12 Jul 2013 16:21:28 +0200 Subject: [PATCH] static-handler: refactor to use write_handler_response This fixes sending out the weak ETag for ngx_pagespeed_static/js_defer.hash.js, and simplifies things somewhat for this code path. It also adds a test for it. Fixes https://github.com/pagespeed/ngx_pagespeed/issues/434 --- src/ngx_pagespeed.cc | 76 +++++++++++---------------------------- test/nginx_system_test.sh | 19 ++++++++++ 2 files changed, 40 insertions(+), 55 deletions(-) diff --git a/src/ngx_pagespeed.cc b/src/ngx_pagespeed.cc index f4293b1ae..66b007ee8 100644 --- a/src/ngx_pagespeed.cc +++ b/src/ngx_pagespeed.cc @@ -91,6 +91,8 @@ char* string_piece_to_pool_string(ngx_pool_t* pool, StringPiece sp) { ngx_uint_t buffer_size = sp.size() + 1; char* s = static_cast(ngx_palloc(pool, buffer_size)); if (s == NULL) { + LOG(ERROR) << "string_piece_to_pool_string: ngx_palloc() returned NULL"; + DCHECK(false); return NULL; } sp.copy(s, buffer_size /* max to copy */); @@ -378,6 +380,12 @@ char* ps_loc_configure(ngx_conf_t* cf, ngx_command_t* cmd, void* conf); void ps_ignore_sigpipe(); +void ps_write_handler_response(const StringPiece& output, + ngx_http_request_t* r, + net_instaweb::ContentType content_type, + const StringPiece& cache_control, + net_instaweb::Timer* timer); + ngx_command_t ps_commands[] = { { ngx_string("pagespeed"), NGX_HTTP_MAIN_CONF|NGX_HTTP_SRV_CONF|NGX_CONF_TAKE1| @@ -2026,8 +2034,6 @@ ngx_int_t ps_header_filter(ngx_http_request_t* r) { return ngx_http_next_header_filter(r); } -// TODO(oschaaf): make ps_static_handler use ps_write_handler_response? for now, -// minimize the diff ngx_int_t ps_static_handler(ngx_http_request_t* r) { ps_srv_conf_t* cfg_s = ps_get_srv_config(r); @@ -2046,59 +2052,9 @@ ngx_int_t ps_static_handler(ngx_http_request_t* r) { return NGX_DECLINED; } - // Set and send headers. - r->headers_out.status = NGX_HTTP_OK; - - // Content length - r->headers_out.content_length_n = file_contents.size(); - - // Content type - StringPiece content_type_sp(content_type.mime_type()); - r->headers_out.content_type_len = content_type_sp.length(); - r->headers_out.content_type.len = content_type_sp.length(); - r->headers_out.content_type.data = reinterpret_cast( - string_piece_to_pool_string(r->pool, content_type_sp)); - if (r->headers_out.content_type.data == NULL) { - return NGX_ERROR; - } - // ngx_http_test_content_type() will recalculate this if we null it - r->headers_out.content_type_lowcase = NULL; - - // Cache control - char* cache_control_s = string_piece_to_pool_string(r->pool, cache_header); - if (cache_control_s == NULL) { - return NGX_ERROR; - } - ps_set_cache_control(r, cache_control_s); - - if (net_instaweb::FindIgnoreCase(cache_header, "private") == - static_cast(StringPiece::npos)) { - ngx_table_elt_t* etag = static_cast( - ngx_list_push(&r->headers_out.headers)); - if (etag == NULL) { - return NGX_ERROR; - } - - etag->hash = 1; // Include this header in the output. - etag->key.len = 4; - etag->key.data = reinterpret_cast(const_cast("ETag")); - etag->value.len = 5; - etag->value.data = reinterpret_cast(const_cast("W/\"0\"")); - r->headers_out.etag = etag; - } - - ngx_http_send_header(r); - - // Send the body. - ngx_chain_t* out; - ngx_int_t rc = string_piece_to_buffer_chain( - r->pool, file_contents, &out, true /* send_last_buf */); - if (rc == NGX_ERROR) { - return NGX_ERROR; - } - CHECK(rc == NGX_OK); - - return ngx_http_output_filter(r, out); + ps_write_handler_response(file_contents, r, content_type, cache_header, + cfg_s->server_context->factory()->timer()); + return NGX_OK; } ngx_int_t send_out_headers_and_body( @@ -2149,6 +2105,16 @@ void ps_write_handler_response(const StringPiece& output, response_headers.SetLastModified(now_ms); response_headers.Add(net_instaweb::HttpAttributes::kCacheControl, cache_control); + + char* cache_control_s = string_piece_to_pool_string(r->pool, cache_control); + if (cache_control_s != NULL) { + if (net_instaweb::FindIgnoreCase(cache_control, "private") == + static_cast(StringPiece::npos)) { + response_headers.Add(net_instaweb::HttpAttributes::kEtag, + "W/\"0\""); + } + } + send_out_headers_and_body(r, response_headers, output.as_string()); } diff --git a/test/nginx_system_test.sh b/test/nginx_system_test.sh index 435ae7273..121817162 100755 --- a/test/nginx_system_test.sh +++ b/test/nginx_system_test.sh @@ -1644,4 +1644,23 @@ start_test keepalive with static resources keepalive_test "keepalive-static.example.com"\ "/ngx_pagespeed_static/js_defer.0.js" "" + +test_filter ngx_pagespeed_static defer js served with correct headers. +# First, determine which hash js_defer is served with. We need a correct hash +# to get it served up with an Etag, which is one of the things we want to test. +URL="$HOSTNAME/mod_pagespeed_example/defer_javascript.html?PageSpeed=on&PageSpeedFilters=defer_javascript" +OUT=$($WGET_DUMP $URL) +HASH=$(echo $OUT \ + | grep --only-matching "/js_defer\\.*\([^.]\)*.js" | cut -d '.' -f 2) + +JS_URL="$HOSTNAME/ngx_pagespeed_static/js_defer.$HASH.js" +JS_HEADERS=$($WGET -O /dev/null -q -S --header='Accept-Encoding: gzip' \ + $JS_URL 2>&1) +check_from "$JS_HEADERS" egrep -qi 'HTTP/1[.]. 200 OK' +check_from "$JS_HEADERS" fgrep -qi 'Content-Encoding: gzip' +check_from "$JS_HEADERS" fgrep -qi 'Vary: Accept-Encoding' +# Nginx's gzip module clears etags, which we don't want. Make sure we have it. +check_from "$JS_HEADERS" egrep -qi 'Etag: W/"0"' +check_from "$JS_HEADERS" fgrep -qi 'Last-Modified:' + check_failures_and_exit