Permalink
Browse files

If we're receiving a response slowly and the user aborts, we need to …

…note this and not record the truncated resource to cache.

Fixes #1081

Nginx side of the change: apache/incubator-pagespeed-ngx#966
  • Loading branch information...
jeffkaufman authored and crowell committed May 15, 2015
1 parent fcfc4f5 commit 7a3fab0d5d517f2dd8808f906f75c50ec2c58f87
@@ -818,7 +818,9 @@ apr_status_t instaweb_in_place_check_headers_filter(ap_filter_t* filter,
// We now have the final headers. If they don't let us cache then we'll
// abort even though we've already buffered up the whole resource.
InstawebHandler::AboutToBeDoneWithRecorder(request, recorder);
recorder->DoneAndSetHeaders(&response_headers); // Deletes recorder.
// Deletes recorder
recorder->DoneAndSetHeaders(&response_headers,
!request->connection->aborted);
}
}

@@ -208,7 +208,13 @@ void InPlaceResourceRecorder::DroppedDueToSize() {
}

void InPlaceResourceRecorder::DoneAndSetHeaders(
ResponseHeaders* response_headers) {
ResponseHeaders* response_headers, bool entire_response_received) {
if (!entire_response_received) {
// To record successfully, we must have a complete response. Otherwise you
// get https://github.com/pagespeed/mod_pagespeed/issues/1081.
Fail();
}

if (!failure_ && !full_response_headers_considered_) {
ConsiderResponseHeaders(kFullHeaders, response_headers);
}
@@ -97,7 +97,8 @@ class InPlaceResourceRecorderTest : public RewriteTestBase {
StringPiece gzipped(reinterpret_cast<const char*>(&kGzippedData[0]),
STATIC_STRLEN(kGzippedData));
recorder->Write(gzipped, message_handler());
recorder.release()->DoneAndSetHeaders(&final_headers);
recorder.release()->DoneAndSetHeaders(
&final_headers, true /* complete response */);

HTTPValue value_out;
ResponseHeaders headers_out;
@@ -160,7 +161,8 @@ TEST_F(InPlaceResourceRecorderTest, BasicOperation) {
InPlaceResourceRecorder::kPreliminaryHeaders, &prelim_headers);
recorder->Write(kHello, message_handler());
recorder->Write(kBye, message_handler());
recorder.release()->DoneAndSetHeaders(&ok_headers);
recorder.release()->DoneAndSetHeaders(
&ok_headers, true /* complete response */);

HTTPValue value_out;
ResponseHeaders headers_out;
@@ -171,6 +173,26 @@ TEST_F(InPlaceResourceRecorderTest, BasicOperation) {
EXPECT_EQ(StrCat(kHello, kBye), contents);
}

TEST_F(InPlaceResourceRecorderTest, IncompleteResponse) {
ResponseHeaders prelim_headers;
prelim_headers.set_status_code(HttpStatus::kOK);

ResponseHeaders ok_headers;
SetDefaultLongCacheHeaders(&kContentTypeCss, &ok_headers);

scoped_ptr<InPlaceResourceRecorder> recorder(MakeRecorder(kTestUrl));
recorder->ConsiderResponseHeaders(
InPlaceResourceRecorder::kPreliminaryHeaders, &prelim_headers);
recorder->Write(kHello, message_handler());
recorder.release()->DoneAndSetHeaders(
&ok_headers, false /* incomplete response */);

HTTPValue value_out;
ResponseHeaders headers_out;
EXPECT_EQ(HTTPCache::kNotFound,
HttpBlockingFind(kTestUrl, http_cache(), &value_out, &headers_out));
}

TEST_F(InPlaceResourceRecorderTest, CheckCacheableContentTypes) {
CheckCacheableContentType(&kContentTypeJpeg);
CheckCacheableContentType(&kContentTypeCss);
@@ -192,7 +214,8 @@ TEST_F(InPlaceResourceRecorderTest, BasicOperationFullHeaders) {
InPlaceResourceRecorder::kFullHeaders, &ok_headers);
recorder->Write(kHello, message_handler());
recorder->Write(kBye, message_handler());
recorder.release()->DoneAndSetHeaders(&ok_headers);
recorder.release()->DoneAndSetHeaders(
&ok_headers, true /* complete response */);

HTTPValue value_out;
ResponseHeaders headers_out;
@@ -216,7 +239,8 @@ TEST_F(InPlaceResourceRecorderTest, DontRemember304) {
scoped_ptr<InPlaceResourceRecorder> recorder(MakeRecorder(kTestUrl));
recorder->ConsiderResponseHeaders(
InPlaceResourceRecorder::kPreliminaryHeaders, &prelim_headers);
recorder.release()->DoneAndSetHeaders(&not_modified_headers);
recorder.release()->DoneAndSetHeaders(
&not_modified_headers, true /* complete response */);

HTTPValue value_out;
ResponseHeaders headers_out;
@@ -237,7 +261,8 @@ TEST_F(InPlaceResourceRecorderTest, Remember500AsFetchFailed) {
scoped_ptr<InPlaceResourceRecorder> recorder(MakeRecorder(kTestUrl));
recorder->ConsiderResponseHeaders(
InPlaceResourceRecorder::kPreliminaryHeaders, &prelim_headers);
recorder.release()->DoneAndSetHeaders(&error_headers);
recorder.release()->DoneAndSetHeaders(
&error_headers, true /* complete response */);

HTTPValue value_out;
ResponseHeaders headers_out;
@@ -252,7 +277,8 @@ TEST_F(InPlaceResourceRecorderTest, RememberEmpty) {

scoped_ptr<InPlaceResourceRecorder> recorder(MakeRecorder(kTestUrl));
// No contents written.
recorder.release()->DoneAndSetHeaders(&ok_headers);
recorder.release()->DoneAndSetHeaders(
&ok_headers, true /* complete response */);

HTTPValue value_out;
ResponseHeaders headers_out;
@@ -109,10 +109,16 @@ class InPlaceResourceRecorder : public Writer {
// Because of Apache's quirky filter order, we cannot get both the
// uncompressed final contents and the complete headers at the same time.
//
// Set entire_response_received to true if you know that the response data fed
// into Write() is complete. For example, if the browser cancelled the
// download and so this is a partial response, set entire_response_received to
// false so we know not to cache it.
//
// Does not take ownership of response_headers.
//
// Deletes itself. Do not use object after calling DoneAndSetHeaders().
void DoneAndSetHeaders(ResponseHeaders* response_headers);
void DoneAndSetHeaders(ResponseHeaders* response_headers,
bool entire_response_received);

const GoogleString& url() const { return url_; }
MessageHandler* handler() { return handler_; }

0 comments on commit 7a3fab0

Please sign in to comment.