diff --git a/net/http/http_network_transaction.cc b/net/http/http_network_transaction.cc index 4c6be63cadf324..ba27cb7e19bb86 100644 --- a/net/http/http_network_transaction.cc +++ b/net/http/http_network_transaction.cc @@ -1443,7 +1443,11 @@ int HttpNetworkTransaction::HandleIOError(int error) { // likely happen when trying to retrieve its IP address. // See http://crbug.com/105824 for more details. case ERR_SOCKET_NOT_CONNECTED: - if (ShouldResendRequest(error)) { + // If a socket is closed on its initial request, HttpStreamParser returns + // ERR_EMPTY_RESPONSE. This may still be close/reuse race if the socket was + // preconnected but failed to be used before the server timed it out. + case ERR_EMPTY_RESPONSE: + if (ShouldResendRequest()) { net_log_.AddEventWithNetErrorCode( NetLog::TYPE_HTTP_TRANSACTION_RESTART_AFTER_ERROR, error); ResetConnectionAndRequestForResend(); @@ -1494,7 +1498,7 @@ HttpResponseHeaders* HttpNetworkTransaction::GetResponseHeaders() const { return response_.headers.get(); } -bool HttpNetworkTransaction::ShouldResendRequest(int error) const { +bool HttpNetworkTransaction::ShouldResendRequest() const { bool connection_is_proven = stream_->IsConnectionReused(); bool has_received_headers = GetResponseHeaders() != NULL; diff --git a/net/http/http_network_transaction.h b/net/http/http_network_transaction.h index b10e155d15acbf..24ab95b8ac21e6 100644 --- a/net/http/http_network_transaction.h +++ b/net/http/http_network_transaction.h @@ -210,9 +210,9 @@ class NET_EXPORT_PRIVATE HttpNetworkTransaction // Gets the response headers from the HttpStream. HttpResponseHeaders* GetResponseHeaders() const; - // Called when we reached EOF or got an error. Returns true if we should - // resend the request. |error| is OK when we reached EOF. - bool ShouldResendRequest(int error) const; + // Called when the socket is unexpectedly closed. Returns true if the request + // should be resent in case of a socket reuse/close race. + bool ShouldResendRequest() const; // Resets the connection and the request headers for resend. Called when // ShouldResendRequest() is true. diff --git a/net/http/http_network_transaction_unittest.cc b/net/http/http_network_transaction_unittest.cc index 0703350ab2ffb3..13cef372d5242c 100644 --- a/net/http/http_network_transaction_unittest.cc +++ b/net/http/http_network_transaction_unittest.cc @@ -16,6 +16,7 @@ #include "base/json/json_writer.h" #include "base/memory/scoped_ptr.h" #include "base/memory/weak_ptr.h" +#include "base/run_loop.h" #include "base/strings/string_util.h" #include "base/strings/utf_string_conversions.h" #include "base/test/test_file_util.h" @@ -283,6 +284,13 @@ class HttpNetworkTransactionTest void KeepAliveConnectionResendRequestTest(const MockWrite* write_failure, const MockRead* read_failure); + // Either |write_failure| specifies a write failure or |read_failure| + // specifies a read failure when using a reused socket. In either case, the + // failure should cause the network transaction to resend the request, and the + // other argument should be NULL. + void PreconnectErrorResendRequestTest(const MockWrite* write_failure, + const MockRead* read_failure); + SimpleGetHelperResult SimpleGetHelperForData(StaticSocketDataProvider* data[], size_t data_count) { SimpleGetHelperResult out; @@ -1239,7 +1247,7 @@ void HttpNetworkTransactionTest::KeepAliveConnectionResendRequestTest( }; if (write_failure) { - ASSERT_TRUE(!read_failure); + ASSERT_FALSE(read_failure); data1_writes[1] = *write_failure; } else { ASSERT_TRUE(read_failure); @@ -1298,6 +1306,90 @@ void HttpNetworkTransactionTest::KeepAliveConnectionResendRequestTest( } } +void HttpNetworkTransactionTest::PreconnectErrorResendRequestTest( + const MockWrite* write_failure, + const MockRead* read_failure) { + HttpRequestInfo request; + request.method = "GET"; + request.url = GURL("http://www.foo.com/"); + request.load_flags = 0; + + CapturingNetLog net_log; + session_deps_.net_log = &net_log; + scoped_refptr session(CreateSession(&session_deps_)); + + // Written data for successfully sending a request. + MockWrite data1_writes[] = { + MockWrite("GET / HTTP/1.1\r\n" + "Host: www.foo.com\r\n" + "Connection: keep-alive\r\n\r\n"), + }; + + // Read results for the first request. + MockRead data1_reads[] = { + MockRead(ASYNC, OK), + }; + + if (write_failure) { + ASSERT_FALSE(read_failure); + data1_writes[0] = *write_failure; + } else { + ASSERT_TRUE(read_failure); + data1_reads[0] = *read_failure; + } + + StaticSocketDataProvider data1(data1_reads, arraysize(data1_reads), + data1_writes, arraysize(data1_writes)); + session_deps_.socket_factory->AddSocketDataProvider(&data1); + + MockRead data2_reads[] = { + MockRead("HTTP/1.1 200 OK\r\nContent-Length: 5\r\n\r\n"), + MockRead("hello"), + MockRead(ASYNC, OK), + }; + StaticSocketDataProvider data2(data2_reads, arraysize(data2_reads), NULL, 0); + session_deps_.socket_factory->AddSocketDataProvider(&data2); + + // Preconnect a socket. + net::SSLConfig ssl_config; + session->ssl_config_service()->GetSSLConfig(&ssl_config); + if (session->http_stream_factory()->has_next_protos()) + ssl_config.next_protos = session->http_stream_factory()->next_protos(); + session->http_stream_factory()->PreconnectStreams( + 1, request, DEFAULT_PRIORITY, ssl_config, ssl_config); + // Wait for the preconnect to complete. + // TODO(davidben): Some way to wait for an idle socket count might be handy. + base::RunLoop().RunUntilIdle(); + EXPECT_EQ(1, GetIdleSocketCountInTransportSocketPool(session.get())); + + // Make the request. + TestCompletionCallback callback; + + scoped_ptr trans( + new HttpNetworkTransaction(DEFAULT_PRIORITY, session.get())); + + int rv = trans->Start(&request, callback.callback(), BoundNetLog()); + EXPECT_EQ(ERR_IO_PENDING, rv); + + rv = callback.WaitForResult(); + EXPECT_EQ(OK, rv); + + LoadTimingInfo load_timing_info; + EXPECT_TRUE(trans->GetLoadTimingInfo(&load_timing_info)); + TestLoadTimingNotReused(load_timing_info, CONNECT_TIMING_HAS_DNS_TIMES); + + const HttpResponseInfo* response = trans->GetResponseInfo(); + ASSERT_TRUE(response != NULL); + + EXPECT_TRUE(response->headers.get() != NULL); + EXPECT_EQ("HTTP/1.1 200 OK", response->headers->GetStatusLine()); + + std::string response_data; + rv = ReadTransaction(trans.get(), &response_data); + EXPECT_EQ(OK, rv); + EXPECT_EQ("hello", response_data); +} + TEST_P(HttpNetworkTransactionTest, KeepAliveConnectionNotConnectedOnWrite) { MockWrite write_failure(ASYNC, ERR_SOCKET_NOT_CONNECTED); @@ -1314,6 +1406,22 @@ TEST_P(HttpNetworkTransactionTest, KeepAliveConnectionEOF) { KeepAliveConnectionResendRequestTest(NULL, &read_failure); } +TEST_P(HttpNetworkTransactionTest, + PreconnectErrorNotConnectedOnWrite) { + MockWrite write_failure(ASYNC, ERR_SOCKET_NOT_CONNECTED); + PreconnectErrorResendRequestTest(&write_failure, NULL); +} + +TEST_P(HttpNetworkTransactionTest, PreconnectErrorReset) { + MockRead read_failure(ASYNC, ERR_CONNECTION_RESET); + PreconnectErrorResendRequestTest(NULL, &read_failure); +} + +TEST_P(HttpNetworkTransactionTest, PreconnectErrorEOF) { + MockRead read_failure(SYNCHRONOUS, OK); // EOF + PreconnectErrorResendRequestTest(NULL, &read_failure); +} + TEST_P(HttpNetworkTransactionTest, NonKeepAliveConnectionReset) { HttpRequestInfo request; request.method = "GET"; diff --git a/net/http/http_pipelined_connection_impl.cc b/net/http/http_pipelined_connection_impl.cc index 2ee9c2944dd0ea..ebd73f24ab7fcf 100644 --- a/net/http/http_pipelined_connection_impl.cc +++ b/net/http/http_pipelined_connection_impl.cc @@ -650,7 +650,7 @@ bool HttpPipelinedConnectionImpl::IsConnectionReused(int pipeline_id) const { void HttpPipelinedConnectionImpl::SetConnectionReused(int pipeline_id) { CHECK(ContainsKey(stream_info_map_, pipeline_id)); - connection_->set_is_reused(true); + connection_->set_reuse_type(ClientSocketHandle::REUSED_IDLE); } int64 HttpPipelinedConnectionImpl::GetTotalReceivedBytes( diff --git a/net/http/http_proxy_client_socket.cc b/net/http/http_proxy_client_socket.cc index 7037616ae5a02b..e2e46660dc2b52 100644 --- a/net/http/http_proxy_client_socket.cc +++ b/net/http/http_proxy_client_socket.cc @@ -276,7 +276,7 @@ int HttpProxyClientSocket::PrepareForAuthRestart() { int HttpProxyClientSocket::DidDrainBodyForAuthRestart(bool keep_alive) { if (keep_alive && transport_->socket()->IsConnectedAndIdle()) { next_state_ = STATE_GENERATE_AUTH_TOKEN; - transport_->set_is_reused(true); + transport_->set_reuse_type(ClientSocketHandle::REUSED_IDLE); } else { // This assumes that the underlying transport socket is a TCP socket, // since only TCP sockets are restartable. diff --git a/net/http/http_stream_parser.cc b/net/http/http_stream_parser.cc index 0821c848652366..9b787dae5f3511 100644 --- a/net/http/http_stream_parser.cc +++ b/net/http/http_stream_parser.cc @@ -904,7 +904,7 @@ bool HttpStreamParser::IsConnectionReused() const { } void HttpStreamParser::SetConnectionReused() { - connection_->set_is_reused(true); + connection_->set_reuse_type(ClientSocketHandle::REUSED_IDLE); } bool HttpStreamParser::IsConnectionReusable() const { diff --git a/net/socket/client_socket_handle.cc b/net/socket/client_socket_handle.cc index e42e9fcada34fb..53bcd77499a0af 100644 --- a/net/socket/client_socket_handle.cc +++ b/net/socket/client_socket_handle.cc @@ -19,7 +19,7 @@ ClientSocketHandle::ClientSocketHandle() : is_initialized_(false), pool_(NULL), higher_pool_(NULL), - is_reused_(false), + reuse_type_(ClientSocketHandle::UNUSED), callback_(base::Bind(&ClientSocketHandle::OnIOComplete, base::Unretained(this))), is_ssl_error_(false) {} @@ -58,7 +58,7 @@ void ClientSocketHandle::ResetInternal(bool cancel) { is_initialized_ = false; socket_.reset(); group_name_.clear(); - is_reused_ = false; + reuse_type_ = ClientSocketHandle::UNUSED; user_callback_.Reset(); if (higher_pool_) RemoveHigherLayeredPool(higher_pool_); diff --git a/net/socket/client_socket_handle.h b/net/socket/client_socket_handle.h index 30b7c03e9dc09e..0899d9a9bb067b 100644 --- a/net/socket/client_socket_handle.h +++ b/net/socket/client_socket_handle.h @@ -126,7 +126,7 @@ class NET_EXPORT ClientSocketHandle { // SetSocket() may also be used if this handle is used as simply for // socket storage (e.g., http://crbug.com/37810). void SetSocket(scoped_ptr s); - void set_is_reused(bool is_reused) { is_reused_ = is_reused; } + void set_reuse_type(SocketReuseType reuse_type) { reuse_type_ = reuse_type; } void set_idle_time(base::TimeDelta idle_time) { idle_time_ = idle_time; } void set_pool_id(int id) { pool_id_ = id; } void set_is_ssl_error(bool is_ssl_error) { is_ssl_error_ = is_ssl_error; } @@ -161,17 +161,9 @@ class NET_EXPORT ClientSocketHandle { // These may only be used if is_initialized() is true. const std::string& group_name() const { return group_name_; } int id() const { return pool_id_; } - bool is_reused() const { return is_reused_; } + bool is_reused() const { return reuse_type_ == REUSED_IDLE; } base::TimeDelta idle_time() const { return idle_time_; } - SocketReuseType reuse_type() const { - if (is_reused()) { - return REUSED_IDLE; - } else if (idle_time() == base::TimeDelta()) { - return UNUSED; - } else { - return UNUSED_IDLE; - } - } + SocketReuseType reuse_type() const { return reuse_type_; } const LoadTimingInfo::ConnectTiming& connect_timing() const { return connect_timing_; } @@ -200,7 +192,7 @@ class NET_EXPORT ClientSocketHandle { HigherLayeredPool* higher_pool_; scoped_ptr socket_; std::string group_name_; - bool is_reused_; + SocketReuseType reuse_type_; CompletionCallback callback_; CompletionCallback user_callback_; base::TimeDelta idle_time_; diff --git a/net/socket/client_socket_pool_base.cc b/net/socket/client_socket_pool_base.cc index 1c79923a40057f..e20b355dafe329 100644 --- a/net/socket/client_socket_pool_base.cc +++ b/net/socket/client_socket_pool_base.cc @@ -15,7 +15,6 @@ #include "base/values.h" #include "net/base/net_errors.h" #include "net/base/net_log.h" -#include "net/socket/client_socket_handle.h" using base::TimeDelta; @@ -401,7 +400,7 @@ int ClientSocketPoolBaseHelper::RequestSocketInternal( if (rv == OK) { LogBoundConnectJobToRequest(connect_job->net_log().source(), request); if (!preconnecting) { - HandOutSocket(connect_job->PassSocket(), false /* not reused */, + HandOutSocket(connect_job->PassSocket(), ClientSocketHandle::UNUSED, connect_job->connect_timing(), handle, base::TimeDelta(), group, request.net_log()); } else { @@ -427,7 +426,7 @@ int ClientSocketPoolBaseHelper::RequestSocketInternal( error_socket = connect_job->PassSocket(); } if (error_socket) { - HandOutSocket(error_socket.Pass(), false /* not reused */, + HandOutSocket(error_socket.Pass(), ClientSocketHandle::UNUSED, connect_job->connect_timing(), handle, base::TimeDelta(), group, request.net_log()); } else if (group->IsEmpty()) { @@ -476,9 +475,16 @@ bool ClientSocketPoolBaseHelper::AssignIdleSocketToRequest( base::TimeTicks::Now() - idle_socket_it->start_time; IdleSocket idle_socket = *idle_socket_it; idle_sockets->erase(idle_socket_it); + // TODO(davidben): If |idle_time| is under some low watermark, consider + // treating as UNUSED rather than UNUSED_IDLE. This will avoid + // HttpNetworkTransaction retrying on some errors. + ClientSocketHandle::SocketReuseType reuse_type = + idle_socket.socket->WasEverUsed() ? + ClientSocketHandle::REUSED_IDLE : + ClientSocketHandle::UNUSED_IDLE; HandOutSocket( scoped_ptr(idle_socket.socket), - idle_socket.socket->WasEverUsed(), + reuse_type, LoadTimingInfo::ConnectTiming(), request.handle(), idle_time, @@ -878,7 +884,7 @@ void ClientSocketPoolBaseHelper::OnConnectJobComplete( if (request) { LogBoundConnectJobToRequest(job_log.source(), *request); HandOutSocket( - socket.Pass(), false /* unused socket */, connect_timing, + socket.Pass(), ClientSocketHandle::UNUSED, connect_timing, request->handle(), base::TimeDelta(), group, request->net_log()); request->net_log().EndEvent(NetLog::TYPE_SOCKET_POOL); InvokeUserCallbackLater(request->handle(), request->callback(), result); @@ -898,7 +904,7 @@ void ClientSocketPoolBaseHelper::OnConnectJobComplete( RemoveConnectJob(job, group); if (socket.get()) { handed_out_socket = true; - HandOutSocket(socket.Pass(), false /* unused socket */, + HandOutSocket(socket.Pass(), ClientSocketHandle::UNUSED, connect_timing, request->handle(), base::TimeDelta(), group, request->net_log()); } @@ -963,7 +969,7 @@ void ClientSocketPoolBaseHelper::ProcessPendingRequest( void ClientSocketPoolBaseHelper::HandOutSocket( scoped_ptr socket, - bool reused, + ClientSocketHandle::SocketReuseType reuse_type, const LoadTimingInfo::ConnectTiming& connect_timing, ClientSocketHandle* handle, base::TimeDelta idle_time, @@ -971,12 +977,12 @@ void ClientSocketPoolBaseHelper::HandOutSocket( const BoundNetLog& net_log) { DCHECK(socket); handle->SetSocket(socket.Pass()); - handle->set_is_reused(reused); + handle->set_reuse_type(reuse_type); handle->set_idle_time(idle_time); handle->set_pool_id(pool_generation_number_); handle->set_connect_timing(connect_timing); - if (reused) { + if (handle->is_reused()) { net_log.AddEvent( NetLog::TYPE_SOCKET_POOL_REUSED_AN_EXISTING_SOCKET, NetLog::IntegerCallback( diff --git a/net/socket/client_socket_pool_base.h b/net/socket/client_socket_pool_base.h index 2c2ddb57abc532..8a0a1f3c102512 100644 --- a/net/socket/client_socket_pool_base.h +++ b/net/socket/client_socket_pool_base.h @@ -46,6 +46,7 @@ #include "net/base/network_change_notifier.h" #include "net/base/priority_queue.h" #include "net/base/request_priority.h" +#include "net/socket/client_socket_handle.h" #include "net/socket/client_socket_pool.h" #include "net/socket/stream_socket.h" @@ -528,7 +529,7 @@ class NET_EXPORT_PRIVATE ClientSocketPoolBaseHelper // Assigns |socket| to |handle| and updates |group|'s counters appropriately. void HandOutSocket(scoped_ptr socket, - bool reused, + ClientSocketHandle::SocketReuseType reuse_type, const LoadTimingInfo::ConnectTiming& connect_timing, ClientSocketHandle* handle, base::TimeDelta time_idle,