From fc8ffd6e8f0ce5fc1753baa37d65dc108f493f7f Mon Sep 17 00:00:00 2001 From: "davidben@chromium.org" Date: Fri, 21 Mar 2014 19:35:28 +0000 Subject: Fix SPDY error-handling if the connection gets closed just after use. - Make SpdySession::IsReused() return true if the underlying socket was UNUSED_IDLE. This makes the HttpNetworkTransaction-level retry try a fresh socket in case a preconnected socket was stale. - If the SpdySession closes in an event loop iteration between HttpStreamFactoryImplJob::DoCreateStream and OnNewSpdySessionReadyCallback, propogate the error to the request to prevent it from hanging. Do so by creating the originating request's SpdyHttpStream as soon as the SpdySession is created so it can sample SpdySession::IsReused() and advise HttpNetworkTransaction on error. - Delay pumping the SpdySession read loop by an event loop iteration. This simplifies some logic and ensures that HttpNetworkTransaction receives a SpdyHttpStream to advise retry logic. This does mean we lose the error code; it now follows the asynchronous case and turns into ERR_CONNECTION_CLOSED in SpdyHttpStream::InitializeStream. BUG=352156 Review URL: https://codereview.chromium.org/200723004 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@258647 0039d316-1c4b-4281-b951-d872f2087c98 --- net/http/http_network_transaction_unittest.cc | 122 ++++++++++++++++++++------ net/http/http_proxy_client_socket_pool.cc | 10 +-- net/http/http_stream_factory_impl_job.cc | 31 ++++--- net/http/http_stream_factory_impl_request.cc | 30 ++++--- net/http/http_stream_factory_impl_request.h | 2 + net/spdy/spdy_session.cc | 40 ++++----- net/spdy/spdy_session.h | 20 ++--- net/spdy/spdy_session_pool.cc | 21 ++--- net/spdy/spdy_session_pool.h | 9 +- net/spdy/spdy_session_unittest.cc | 17 ++-- net/spdy/spdy_test_util_common.cc | 40 ++++----- net/spdy/spdy_test_util_common.h | 15 ++-- 12 files changed, 213 insertions(+), 144 deletions(-) diff --git a/net/http/http_network_transaction_unittest.cc b/net/http/http_network_transaction_unittest.cc index 7dda13e..ca06785 100644 --- a/net/http/http_network_transaction_unittest.cc +++ b/net/http/http_network_transaction_unittest.cc @@ -289,7 +289,8 @@ class HttpNetworkTransactionTest // 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); + const MockRead* read_failure, + bool use_spdy); SimpleGetHelperResult SimpleGetHelperForData(StaticSocketDataProvider* data[], size_t data_count) { @@ -1308,46 +1309,81 @@ void HttpNetworkTransactionTest::KeepAliveConnectionResendRequestTest( void HttpNetworkTransactionTest::PreconnectErrorResendRequestTest( const MockWrite* write_failure, - const MockRead* read_failure) { + const MockRead* read_failure, + bool use_spdy) { HttpRequestInfo request; request.method = "GET"; - request.url = GURL("http://www.foo.com/"); + request.url = GURL("https://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"), - }; + SSLSocketDataProvider ssl1(ASYNC, OK); + SSLSocketDataProvider ssl2(ASYNC, OK); + if (use_spdy) { + ssl1.SetNextProto(GetParam()); + ssl2.SetNextProto(GetParam()); + } + session_deps_.socket_factory->AddSSLSocketDataProvider(&ssl1); + session_deps_.socket_factory->AddSSLSocketDataProvider(&ssl2); - // Read results for the first request. - MockRead data1_reads[] = { - MockRead(ASYNC, OK), - }; + // SPDY versions of the request and response. + scoped_ptr spdy_request(spdy_util_.ConstructSpdyGet( + request.url.spec().c_str(), false, 1, DEFAULT_PRIORITY)); + scoped_ptr spdy_response( + spdy_util_.ConstructSpdyGetSynReply(NULL, 0, 1)); + scoped_ptr spdy_data( + spdy_util_.ConstructSpdyBodyFrame(1, "hello", 5, true)); + // HTTP/1.1 versions of the request and response. + const char kHttpRequest[] = "GET / HTTP/1.1\r\n" + "Host: www.foo.com\r\n" + "Connection: keep-alive\r\n\r\n"; + const char kHttpResponse[] = "HTTP/1.1 200 OK\r\nContent-Length: 5\r\n\r\n"; + const char kHttpData[] = "hello"; + + std::vector data1_reads; + std::vector data1_writes; if (write_failure) { ASSERT_FALSE(read_failure); - data1_writes[0] = *write_failure; + data1_writes.push_back(*write_failure); + data1_reads.push_back(MockRead(ASYNC, OK)); } else { ASSERT_TRUE(read_failure); - data1_reads[0] = *read_failure; + if (use_spdy) { + data1_writes.push_back(CreateMockWrite(*spdy_request)); + } else { + data1_writes.push_back(MockWrite(kHttpRequest)); + } + data1_reads.push_back(*read_failure); } - StaticSocketDataProvider data1(data1_reads, arraysize(data1_reads), - data1_writes, arraysize(data1_writes)); + StaticSocketDataProvider data1(&data1_reads[0], data1_reads.size(), + &data1_writes[0], data1_writes.size()); 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); + std::vector data2_reads; + std::vector data2_writes; + + if (use_spdy) { + data2_writes.push_back(CreateMockWrite(*spdy_request, 0, ASYNC)); + + data2_reads.push_back(CreateMockRead(*spdy_response, 1, ASYNC)); + data2_reads.push_back(CreateMockRead(*spdy_data, 2, ASYNC)); + data2_reads.push_back(MockRead(ASYNC, OK, 3)); + } else { + data2_writes.push_back( + MockWrite(ASYNC, kHttpRequest, strlen(kHttpRequest), 0)); + + data2_reads.push_back( + MockRead(ASYNC, kHttpResponse, strlen(kHttpResponse), 1)); + data2_reads.push_back(MockRead(ASYNC, kHttpData, strlen(kHttpData), 2)); + data2_reads.push_back(MockRead(ASYNC, OK, 3)); + } + OrderedSocketData data2(&data2_reads[0], data2_reads.size(), + &data2_writes[0], data2_writes.size()); session_deps_.socket_factory->AddSocketDataProvider(&data2); // Preconnect a socket. @@ -1360,7 +1396,7 @@ void HttpNetworkTransactionTest::PreconnectErrorResendRequestTest( // 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())); + EXPECT_EQ(1, GetIdleSocketCountInSSLSocketPool(session.get())); // Make the request. TestCompletionCallback callback; @@ -1376,7 +1412,9 @@ void HttpNetworkTransactionTest::PreconnectErrorResendRequestTest( LoadTimingInfo load_timing_info; EXPECT_TRUE(trans->GetLoadTimingInfo(&load_timing_info)); - TestLoadTimingNotReused(load_timing_info, CONNECT_TIMING_HAS_DNS_TIMES); + TestLoadTimingNotReused( + load_timing_info, + CONNECT_TIMING_HAS_DNS_TIMES|CONNECT_TIMING_HAS_SSL_TIMES); const HttpResponseInfo* response = trans->GetResponseInfo(); ASSERT_TRUE(response != NULL); @@ -1387,7 +1425,7 @@ void HttpNetworkTransactionTest::PreconnectErrorResendRequestTest( std::string response_data; rv = ReadTransaction(trans.get(), &response_data); EXPECT_EQ(OK, rv); - EXPECT_EQ("hello", response_data); + EXPECT_EQ(kHttpData, response_data); } TEST_P(HttpNetworkTransactionTest, @@ -1409,17 +1447,43 @@ TEST_P(HttpNetworkTransactionTest, KeepAliveConnectionEOF) { TEST_P(HttpNetworkTransactionTest, PreconnectErrorNotConnectedOnWrite) { MockWrite write_failure(ASYNC, ERR_SOCKET_NOT_CONNECTED); - PreconnectErrorResendRequestTest(&write_failure, NULL); + PreconnectErrorResendRequestTest(&write_failure, NULL, false); } TEST_P(HttpNetworkTransactionTest, PreconnectErrorReset) { MockRead read_failure(ASYNC, ERR_CONNECTION_RESET); - PreconnectErrorResendRequestTest(NULL, &read_failure); + PreconnectErrorResendRequestTest(NULL, &read_failure, false); } TEST_P(HttpNetworkTransactionTest, PreconnectErrorEOF) { MockRead read_failure(SYNCHRONOUS, OK); // EOF - PreconnectErrorResendRequestTest(NULL, &read_failure); + PreconnectErrorResendRequestTest(NULL, &read_failure, false); +} + +TEST_P(HttpNetworkTransactionTest, PreconnectErrorAsyncEOF) { + MockRead read_failure(ASYNC, OK); // EOF + PreconnectErrorResendRequestTest(NULL, &read_failure, false); +} + +TEST_P(HttpNetworkTransactionTest, + SpdyPreconnectErrorNotConnectedOnWrite) { + MockWrite write_failure(ASYNC, ERR_SOCKET_NOT_CONNECTED); + PreconnectErrorResendRequestTest(&write_failure, NULL, true); +} + +TEST_P(HttpNetworkTransactionTest, SpdyPreconnectErrorReset) { + MockRead read_failure(ASYNC, ERR_CONNECTION_RESET); + PreconnectErrorResendRequestTest(NULL, &read_failure, true); +} + +TEST_P(HttpNetworkTransactionTest, SpdyPreconnectErrorEOF) { + MockRead read_failure(SYNCHRONOUS, OK); // EOF + PreconnectErrorResendRequestTest(NULL, &read_failure, true); +} + +TEST_P(HttpNetworkTransactionTest, SpdyPreconnectErrorAsyncEOF) { + MockRead read_failure(ASYNC, OK); // EOF + PreconnectErrorResendRequestTest(NULL, &read_failure, true); } TEST_P(HttpNetworkTransactionTest, NonKeepAliveConnectionReset) { diff --git a/net/http/http_proxy_client_socket_pool.cc b/net/http/http_proxy_client_socket_pool.cc index 8d691b7..e2529f8 100644 --- a/net/http/http_proxy_client_socket_pool.cc +++ b/net/http/http_proxy_client_socket_pool.cc @@ -315,11 +315,11 @@ int HttpProxyConnectJob::DoSpdyProxyCreateStream() { } } else { // Create a session direct to the proxy itself - int rv = spdy_pool->CreateAvailableSessionFromSocket( - key, transport_socket_handle_.Pass(), - net_log(), OK, &spdy_session, /*using_ssl_*/ true); - if (rv < 0) - return rv; + spdy_session = + spdy_pool->CreateAvailableSessionFromSocket( + key, transport_socket_handle_.Pass(), + net_log(), OK, /*using_ssl_*/ true); + DCHECK(spdy_session); } next_state_ = STATE_SPDY_PROXY_CREATE_STREAM_COMPLETE; diff --git a/net/http/http_stream_factory_impl_job.cc b/net/http/http_stream_factory_impl_job.cc index 960bc58..d595f2a 100644 --- a/net/http/http_stream_factory_impl_job.cc +++ b/net/http/http_stream_factory_impl_job.cc @@ -334,20 +334,23 @@ void HttpStreamFactoryImpl::Job::OnWebSocketHandshakeStreamReadyCallback() { } void HttpStreamFactoryImpl::Job::OnNewSpdySessionReadyCallback() { - DCHECK(!stream_.get()); + DCHECK(stream_.get()); DCHECK(!IsPreconnecting()); DCHECK(using_spdy()); - if (!new_spdy_session_) - return; + // Note: an event loop iteration has passed, so |new_spdy_session_| may be + // NULL at this point if the SpdySession closed immediately after creation. base::WeakPtr spdy_session = new_spdy_session_; new_spdy_session_.reset(); if (IsOrphaned()) { - stream_factory_->OnNewSpdySessionReady( - spdy_session, spdy_session_direct_, server_ssl_config_, proxy_info_, - was_npn_negotiated(), protocol_negotiated(), using_spdy(), net_log_); + if (spdy_session) { + stream_factory_->OnNewSpdySessionReady( + spdy_session, spdy_session_direct_, server_ssl_config_, proxy_info_, + was_npn_negotiated(), protocol_negotiated(), using_spdy(), net_log_); + } stream_factory_->OnOrphanedJobComplete(this); } else { - request_->OnNewSpdySessionReady(this, spdy_session, spdy_session_direct_); + request_->OnNewSpdySessionReady( + this, stream_.Pass(), spdy_session, spdy_session_direct_); } // |this| may be deleted after this call. } @@ -1104,21 +1107,27 @@ int HttpStreamFactoryImpl::Job::DoCreateStream() { SpdySessionPool* spdy_pool = session_->spdy_session_pool(); spdy_session = spdy_pool->FindAvailableSession(spdy_session_key, net_log_); if (!spdy_session) { - int error = + new_spdy_session_ = spdy_pool->CreateAvailableSessionFromSocket(spdy_session_key, connection_.Pass(), net_log_, spdy_certificate_error_, - &new_spdy_session_, using_ssl_); - if (error != OK) - return error; const HostPortPair& host_port_pair = spdy_session_key.host_port_pair(); base::WeakPtr http_server_properties = session_->http_server_properties(); if (http_server_properties) http_server_properties->SetSupportsSpdy(host_port_pair, true); spdy_session_direct_ = direct; + + // Create a SpdyHttpStream attached to the session; + // OnNewSpdySessionReadyCallback is not called until an event loop + // iteration later, so if the SpdySession is closed between then, allow + // reuse state from the underlying socket, sampled by SpdyHttpStream, + // bubble up to the request. + bool use_relative_url = direct || request_info_.url.SchemeIs("https"); + stream_.reset(new SpdyHttpStream(new_spdy_session_, use_relative_url)); + return OK; } } diff --git a/net/http/http_stream_factory_impl_request.cc b/net/http/http_stream_factory_impl_request.cc index ff66f14..5f129ac 100644 --- a/net/http/http_stream_factory_impl_request.cc +++ b/net/http/http_stream_factory_impl_request.cc @@ -291,11 +291,16 @@ HttpStreamFactoryImpl::Request::RemoveRequestFromHttpPipeliningRequestMap() { void HttpStreamFactoryImpl::Request::OnNewSpdySessionReady( Job* job, + scoped_ptr stream, const base::WeakPtr& spdy_session, bool direct) { DCHECK(job); DCHECK(job->using_spdy()); + // Note: |spdy_session| may be NULL. In that case, |delegate_| should still + // receive |stream| so the error propogates up correctly, however there is no + // point in broadcasting |spdy_session| to other requests. + // The first case is the usual case. if (!bound_job_.get()) { OrphanJobsExcept(job); @@ -322,21 +327,20 @@ void HttpStreamFactoryImpl::Request::OnNewSpdySessionReady( // implemented. NOTREACHED(); } else { - bool use_relative_url = direct || url().SchemeIs("https"); - delegate_->OnStreamReady( - job->server_ssl_config(), - job->proxy_info(), - new SpdyHttpStream(spdy_session, use_relative_url)); + delegate_->OnStreamReady(job->server_ssl_config(), job->proxy_info(), + stream.release()); } // |this| may be deleted after this point. - factory->OnNewSpdySessionReady(spdy_session, - direct, - used_ssl_config, - used_proxy_info, - was_npn_negotiated, - protocol_negotiated, - using_spdy, - net_log); + if (spdy_session) { + factory->OnNewSpdySessionReady(spdy_session, + direct, + used_ssl_config, + used_proxy_info, + was_npn_negotiated, + protocol_negotiated, + using_spdy, + net_log); + } } void HttpStreamFactoryImpl::Request::OrphanJobsExcept(Job* job) { diff --git a/net/http/http_stream_factory_impl_request.h b/net/http/http_stream_factory_impl_request.h index 3d3b2c8..22c8ff2 100644 --- a/net/http/http_stream_factory_impl_request.h +++ b/net/http/http_stream_factory_impl_request.h @@ -16,6 +16,7 @@ namespace net { class ClientSocketHandle; +class HttpStream; class SpdySession; class HttpStreamFactoryImpl::Request : public HttpStreamRequest { @@ -64,6 +65,7 @@ class HttpStreamFactoryImpl::Request : public HttpStreamRequest { // Called by an attached Job if it sets up a SpdySession. void OnNewSpdySessionReady(Job* job, + scoped_ptr stream, const base::WeakPtr& spdy_session, bool direct); diff --git a/net/spdy/spdy_session.cc b/net/spdy/spdy_session.cc index c20d89e..5f0e285 100644 --- a/net/spdy/spdy_session.cc +++ b/net/spdy/spdy_session.cc @@ -532,7 +532,7 @@ SpdySession::~SpdySession() { net_log_.EndEvent(NetLog::TYPE_SPDY_SESSION); } -Error SpdySession::InitializeWithSocket( +void SpdySession::InitializeWithSocket( scoped_ptr connection, SpdySessionPool* pool, bool is_secure, @@ -593,19 +593,17 @@ Error SpdySession::InitializeWithSocket( NetLog::TYPE_SPDY_SESSION_INITIALIZED, connection_->socket()->NetLog().source().ToEventParametersCallback()); - int error = DoReadLoop(READ_STATE_DO_READ, OK); - if (error == ERR_IO_PENDING) - error = OK; - if (error == OK) { - DCHECK_NE(availability_state_, STATE_CLOSED); - connection_->AddHigherLayeredPool(this); - if (enable_sending_initial_data_) - SendInitialData(); - pool_ = pool; - } else { - DcheckClosed(); - } - return static_cast(error); + DCHECK_NE(availability_state_, STATE_CLOSED); + connection_->AddHigherLayeredPool(this); + if (enable_sending_initial_data_) + SendInitialData(); + pool_ = pool; + + // Bootstrap the read loop. + base::MessageLoop::current()->PostTask( + FROM_HERE, + base::Bind(&SpdySession::PumpReadLoop, + weak_factory_.GetWeakPtr(), READ_STATE_DO_READ, OK)); } bool SpdySession::VerifyDomainAuthentication(const std::string& domain) { @@ -1554,9 +1552,8 @@ SpdySession::CloseSessionResult SpdySession::DoCloseSession( UMA_HISTOGRAM_CUSTOM_COUNTS("Net.SpdySession.BytesRead.OtherErrors", total_bytes_received_, 1, 100000000, 50); - // |pool_| will be NULL when |InitializeWithSocket()| is in the - // call stack. - if (pool_ && availability_state_ != STATE_GOING_AWAY) + DCHECK(pool_); + if (availability_state_ != STATE_GOING_AWAY) pool_->MakeSessionUnavailable(GetWeakPtr()); availability_state_ = STATE_CLOSED; @@ -1628,10 +1625,8 @@ void SpdySession::CloseSessionOnError(Error err, void SpdySession::MakeUnavailable() { if (availability_state_ < STATE_GOING_AWAY) { availability_state_ = STATE_GOING_AWAY; - // |pool_| will be NULL when |InitializeWithSocket()| is in the - // call stack. - if (pool_) - pool_->MakeSessionUnavailable(GetWeakPtr()); + DCHECK(pool_); + pool_->MakeSessionUnavailable(GetWeakPtr()); } } @@ -1686,7 +1681,8 @@ base::Value* SpdySession::GetInfoAsValue() const { } bool SpdySession::IsReused() const { - return buffered_spdy_framer_->frames_received() > 0; + return buffered_spdy_framer_->frames_received() > 0 || + connection_->reuse_type() == ClientSocketHandle::UNUSED_IDLE; } bool SpdySession::GetLoadTimingInfo(SpdyStreamId stream_id, diff --git a/net/spdy/spdy_session.h b/net/spdy/spdy_session.h index cbb87bf..203f1ef 100644 --- a/net/spdy/spdy_session.h +++ b/net/spdy/spdy_session.h @@ -256,13 +256,13 @@ class NET_EXPORT SpdySession : public BufferedSpdyFramerVisitorInterface, // |certificate_error_code| must either be OK or less than // ERR_IO_PENDING. // - // Returns OK on success, or an error on failure. Never returns - // ERR_IO_PENDING. If an error is returned, the session must be - // destroyed immediately. - Error InitializeWithSocket(scoped_ptr connection, - SpdySessionPool* pool, - bool is_secure, - int certificate_error_code); + // The session begins reading from |connection| on a subsequent event loop + // iteration, so the SpdySession may close immediately afterwards if the first + // read of |connection| fails. + void InitializeWithSocket(scoped_ptr connection, + SpdySessionPool* pool, + bool is_secure, + int certificate_error_code); // Returns the protocol used by this session. Always between // kProtoSPDYMinimumVersion and kProtoSPDYMaximumVersion. @@ -367,7 +367,8 @@ class NET_EXPORT SpdySession : public BufferedSpdyFramerVisitorInterface, base::Value* GetInfoAsValue() const; // Indicates whether the session is being reused after having successfully - // used to send/receive data in the past. + // used to send/receive data in the past or if the underlying socket was idle + // before being used for a SPDY session. bool IsReused() const; // Returns true if the underlying transport socket ever had any reads or @@ -617,8 +618,7 @@ class NET_EXPORT SpdySession : public BufferedSpdyFramerVisitorInterface, // Advance the ReadState state machine. |expected_read_state| is the // expected starting read state. // - // This function must always be called via PumpReadLoop() except for - // from InitializeWithSocket(). + // This function must always be called via PumpReadLoop(). int DoReadLoop(ReadState expected_read_state, int result); // The implementations of the states of the ReadState state machine. int DoRead(); diff --git a/net/spdy/spdy_session_pool.cc b/net/spdy/spdy_session_pool.cc index 093db1e..6ce0a93 100644 --- a/net/spdy/spdy_session_pool.cc +++ b/net/spdy/spdy_session_pool.cc @@ -77,12 +77,11 @@ SpdySessionPool::~SpdySessionPool() { CertDatabase::GetInstance()->RemoveObserver(this); } -net::Error SpdySessionPool::CreateAvailableSessionFromSocket( +base::WeakPtr SpdySessionPool::CreateAvailableSessionFromSocket( const SpdySessionKey& key, scoped_ptr connection, const BoundNetLog& net_log, int certificate_error_code, - base::WeakPtr* available_session, bool is_secure) { DCHECK_GE(default_protocol_, kProtoSPDYMinimumVersion); DCHECK_LE(default_protocol_, kProtoSPDYMaximumVersion); @@ -105,22 +104,16 @@ net::Error SpdySessionPool::CreateAvailableSessionFromSocket( trusted_spdy_proxy_, net_log.net_log())); - Error error = new_session->InitializeWithSocket( + new_session->InitializeWithSocket( connection.Pass(), this, is_secure, certificate_error_code); - DCHECK_NE(error, ERR_IO_PENDING); - if (error != OK) { - available_session->reset(); - return error; - } - - *available_session = new_session->GetWeakPtr(); + base::WeakPtr available_session = new_session->GetWeakPtr(); sessions_.insert(new_session.release()); - MapKeyToAvailableSession(key, *available_session); + MapKeyToAvailableSession(key, available_session); net_log.AddEvent( NetLog::TYPE_SPDY_SESSION_POOL_IMPORTED_SESSION_FROM_SOCKET, - (*available_session)->net_log().source().ToEventParametersCallback()); + available_session->net_log().source().ToEventParametersCallback()); // Look up the IP address for this session so that we can match // future sessions (potentially to different domains) which can @@ -129,11 +122,11 @@ net::Error SpdySessionPool::CreateAvailableSessionFromSocket( // to see if this is a direct connection. if (key.proxy_server().is_direct()) { IPEndPoint address; - if ((*available_session)->GetPeerAddress(&address) == OK) + if (available_session->GetPeerAddress(&address) == OK) aliases_[address] = key; } - return error; + return available_session; } base::WeakPtr SpdySessionPool::FindAvailableSession( diff --git a/net/spdy/spdy_session_pool.h b/net/spdy/spdy_session_pool.h index c44e9de..0fad5d2 100644 --- a/net/spdy/spdy_session_pool.h +++ b/net/spdy/spdy_session_pool.h @@ -79,15 +79,14 @@ class NET_EXPORT SpdySessionPool // encountered when connecting the SSL socket, with OK meaning there // was no error. // - // If successful, OK is returned and |available_session| will be - // non-NULL and available. Otherwise, an error is returned and - // |available_session| will be NULL. - net::Error CreateAvailableSessionFromSocket( + // Returns the new SpdySession. Note that the SpdySession begins reading from + // |connection| on a subsequent event loop iteration, so it may be closed + // immediately afterwards if the first read of |connection| fails. + base::WeakPtr CreateAvailableSessionFromSocket( const SpdySessionKey& key, scoped_ptr connection, const BoundNetLog& net_log, int certificate_error_code, - base::WeakPtr* available_session, bool is_secure); // Find an available session for the given key, or NULL if there isn't one. diff --git a/net/spdy/spdy_session_unittest.cc b/net/spdy/spdy_session_unittest.cc index 0e8b370..a5d9292 100644 --- a/net/spdy/spdy_session_unittest.cc +++ b/net/spdy/spdy_session_unittest.cc @@ -190,8 +190,12 @@ INSTANTIATE_TEST_CASE_P( TEST_P(SpdySessionTest, InitialReadError) { CreateDeterministicNetworkSession(); - TryCreateFakeSpdySessionExpectingFailure( + base::WeakPtr session = TryCreateFakeSpdySessionExpectingFailure( spdy_session_pool_, key_, ERR_FAILED); + EXPECT_TRUE(session); + // Flush the read. + base::RunLoop().RunUntilIdle(); + EXPECT_FALSE(session); } namespace { @@ -277,8 +281,6 @@ TEST_P(SpdySessionTest, PendingStreamCancellingAnother) { session->CloseSessionOnError(ERR_ABORTED, "Aborting session"); EXPECT_EQ(ERR_ABORTED, callback1.WaitForResult()); - - data.RunFor(1); } // A session receiving a GOAWAY frame with no active streams should @@ -330,9 +332,12 @@ TEST_P(SpdySessionTest, GoAwayImmediatelyWithNoActiveStreams) { data.StopAfter(1); - TryCreateInsecureSpdySessionExpectingFailure( - http_session_, key_, ERR_CONNECTION_CLOSED, BoundNetLog()); + base::WeakPtr session = + TryCreateInsecureSpdySessionExpectingFailure( + http_session_, key_, ERR_CONNECTION_CLOSED, BoundNetLog()); + base::RunLoop().RunUntilIdle(); + EXPECT_FALSE(session); EXPECT_FALSE(HasSpdySession(spdy_session_pool_, key_)); } @@ -1045,8 +1050,6 @@ TEST_P(SpdySessionTest, FailedPing) { EXPECT_TRUE(session == NULL); EXPECT_FALSE(HasSpdySession(spdy_session_pool_, key_)); - - data.RunFor(1); EXPECT_EQ(NULL, spdy_stream1.get()); } diff --git a/net/spdy/spdy_test_util_common.cc b/net/spdy/spdy_test_util_common.cc index 66a6f8c3..22ff5f6 100644 --- a/net/spdy/spdy_test_util_common.cc +++ b/net/spdy/spdy_test_util_common.cc @@ -540,15 +540,12 @@ base::WeakPtr CreateSpdySessionHelper( EXPECT_EQ(OK, rv); - base::WeakPtr spdy_session; - EXPECT_EQ( - expected_status, + base::WeakPtr spdy_session = http_session->spdy_session_pool()->CreateAvailableSessionFromSocket( - key, connection.Pass(), net_log, OK, &spdy_session, - is_secure)); - EXPECT_EQ(expected_status == OK, spdy_session != NULL); - EXPECT_EQ(expected_status == OK, - HasSpdySession(http_session->spdy_session_pool(), key)); + key, connection.Pass(), net_log, OK, is_secure); + // Failure is reported asynchronously. + EXPECT_TRUE(spdy_session != NULL); + EXPECT_TRUE(HasSpdySession(http_session->spdy_session_pool(), key)); return spdy_session; } @@ -562,14 +559,14 @@ base::WeakPtr CreateInsecureSpdySession( OK, false /* is_secure */); } -void TryCreateInsecureSpdySessionExpectingFailure( +base::WeakPtr TryCreateInsecureSpdySessionExpectingFailure( const scoped_refptr& http_session, const SpdySessionKey& key, Error expected_error, const BoundNetLog& net_log) { DCHECK_LT(expected_error, ERR_IO_PENDING); - CreateSpdySessionHelper(http_session, key, net_log, - expected_error, false /* is_secure */); + return CreateSpdySessionHelper(http_session, key, net_log, + expected_error, false /* is_secure */); } base::WeakPtr CreateSecureSpdySession( @@ -643,17 +640,15 @@ base::WeakPtr CreateFakeSpdySessionHelper( Error expected_status) { EXPECT_NE(expected_status, ERR_IO_PENDING); EXPECT_FALSE(HasSpdySession(pool, key)); - base::WeakPtr spdy_session; scoped_ptr handle(new ClientSocketHandle()); handle->SetSocket(scoped_ptr(new FakeSpdySessionClientSocket( expected_status == OK ? ERR_IO_PENDING : expected_status))); - EXPECT_EQ( - expected_status, + base::WeakPtr spdy_session = pool->CreateAvailableSessionFromSocket( - key, handle.Pass(), BoundNetLog(), OK, &spdy_session, - true /* is_secure */)); - EXPECT_EQ(expected_status == OK, spdy_session != NULL); - EXPECT_EQ(expected_status == OK, HasSpdySession(pool, key)); + key, handle.Pass(), BoundNetLog(), OK, true /* is_secure */); + // Failure is reported asynchronously. + EXPECT_TRUE(spdy_session != NULL); + EXPECT_TRUE(HasSpdySession(pool, key)); return spdy_session; } @@ -664,11 +659,12 @@ base::WeakPtr CreateFakeSpdySession(SpdySessionPool* pool, return CreateFakeSpdySessionHelper(pool, key, OK); } -void TryCreateFakeSpdySessionExpectingFailure(SpdySessionPool* pool, - const SpdySessionKey& key, - Error expected_error) { +base::WeakPtr TryCreateFakeSpdySessionExpectingFailure( + SpdySessionPool* pool, + const SpdySessionKey& key, + Error expected_error) { DCHECK_LT(expected_error, ERR_IO_PENDING); - CreateFakeSpdySessionHelper(pool, key, expected_error); + return CreateFakeSpdySessionHelper(pool, key, expected_error); } SpdySessionPoolPeer::SpdySessionPoolPeer(SpdySessionPool* pool) : pool_(pool) { diff --git a/net/spdy/spdy_test_util_common.h b/net/spdy/spdy_test_util_common.h index 42b595b..668a7e8 100644 --- a/net/spdy/spdy_test_util_common.h +++ b/net/spdy/spdy_test_util_common.h @@ -247,8 +247,9 @@ base::WeakPtr CreateInsecureSpdySession( // Tries to create a SPDY session for the given key but expects the // attempt to fail with the given error. A SPDY session for |key| must -// not already exist. -void TryCreateInsecureSpdySessionExpectingFailure( +// not already exist. The session will be created but close in the +// next event loop iteration. +base::WeakPtr TryCreateInsecureSpdySessionExpectingFailure( const scoped_refptr& http_session, const SpdySessionKey& key, Error expected_error, @@ -269,10 +270,12 @@ base::WeakPtr CreateFakeSpdySession(SpdySessionPool* pool, // Tries to create an insecure SPDY session for the given key but // expects the attempt to fail with the given error. The session will // neither receive nor send any data. A SPDY session for |key| must -// not already exist. -void TryCreateFakeSpdySessionExpectingFailure(SpdySessionPool* pool, - const SpdySessionKey& key, - Error expected_error); +// not already exist. The session will be created but close in the +// next event loop iteration. +base::WeakPtr TryCreateFakeSpdySessionExpectingFailure( + SpdySessionPool* pool, + const SpdySessionKey& key, + Error expected_error); class SpdySessionPoolPeer { public: -- cgit v1.1