From e3861ca766137c03543b2aa9b115bc7ef5af57ab Mon Sep 17 00:00:00 2001 From: "akalin@chromium.org" Date: Sat, 2 Mar 2013 01:00:45 +0000 Subject: [SPDY] Create SpdyStreamRequest class to help manage delayed creation of streams This avoids passing around pointers to memory locations that are asynchronously filled, which is scary. Rework how SpdySession manages its pending stream requests. In particular, avoid leaking streams if a pending request is cancelled before the created stream is handed off to it. Also, replace a list-based queue with an easier to manage deque. Move StreamReleaserCallback to spdy_test_util_common.{h,cc}. BUG=178943 Review URL: https://codereview.chromium.org/12380005 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@185640 0039d316-1c4b-4281-b951-d872f2087c98 --- net/spdy/spdy_http_stream.cc | 23 +- net/spdy/spdy_http_stream.h | 3 + .../spdy_proxy_client_socket_spdy2_unittest.cc | 8 +- .../spdy_proxy_client_socket_spdy3_unittest.cc | 8 +- net/spdy/spdy_session.cc | 293 ++++++++++++--------- net/spdy/spdy_session.h | 156 +++++++---- net/spdy/spdy_session_spdy2_unittest.cc | 275 ++++++++----------- net/spdy/spdy_session_spdy3_unittest.cc | 293 +++++++++------------ net/spdy/spdy_stream_spdy2_unittest.cc | 25 +- net/spdy/spdy_stream_spdy3_unittest.cc | 34 +-- net/spdy/spdy_test_util_common.cc | 40 ++- net/spdy/spdy_test_util_common.h | 39 +++ net/spdy/spdy_websocket_stream.cc | 12 +- net/spdy/spdy_websocket_stream.h | 2 + 14 files changed, 639 insertions(+), 572 deletions(-) (limited to 'net/spdy') diff --git a/net/spdy/spdy_http_stream.cc b/net/spdy/spdy_http_stream.cc index ac4a3bf..9a83ffa 100644 --- a/net/spdy/spdy_http_stream.cc +++ b/net/spdy/spdy_http_stream.cc @@ -69,9 +69,16 @@ int SpdyHttpStream::InitializeStream(const HttpRequestInfo* request_info, if (stream_.get()) return OK; - return spdy_session_->CreateStream(request_info_->url, - request_info_->priority, &stream_, - stream_net_log, callback); + int rv = stream_request_.StartRequest( + spdy_session_, request_info_->url, request_info_->priority, + stream_net_log, + base::Bind(&SpdyHttpStream::OnStreamCreated, + weak_factory_.GetWeakPtr(), callback)); + + if (rv == OK) + stream_ = stream_request_.ReleaseStream(); + + return rv; } const HttpResponseInfo* SpdyHttpStream::GetResponseInfo() const { @@ -274,13 +281,19 @@ int SpdyHttpStream::SendRequest(const HttpRequestHeaders& request_headers, } void SpdyHttpStream::Cancel() { - if (spdy_session_) - spdy_session_->CancelPendingCreateStreams(&stream_); callback_.Reset(); if (stream_) stream_->Cancel(); } +void SpdyHttpStream::OnStreamCreated( + const CompletionCallback& callback, + int rv) { + if (rv == OK) + stream_ = stream_request_.ReleaseStream(); + callback.Run(rv); +} + int SpdyHttpStream::SendData() { CHECK(request_info_ && request_info_->upload_data_stream); CHECK_EQ(0, request_body_buf_->BytesRemaining()); diff --git a/net/spdy/spdy_http_stream.h b/net/spdy/spdy_http_stream.h index 5e2e55e..6968a9f 100644 --- a/net/spdy/spdy_http_stream.h +++ b/net/spdy/spdy_http_stream.h @@ -82,6 +82,8 @@ class NET_EXPORT_PRIVATE SpdyHttpStream : public SpdyStream::Delegate, virtual void OnClose(int status) OVERRIDE; private: + void OnStreamCreated(const CompletionCallback& callback, int rv); + // Reads the data (whether chunked or not) from the request body stream and // sends the data by calling WriteStreamData on the underlying SpdyStream. int SendData(); @@ -98,6 +100,7 @@ class NET_EXPORT_PRIVATE SpdyHttpStream : public SpdyStream::Delegate, bool ShouldWaitForMoreBufferedData() const; base::WeakPtrFactory weak_factory_; + SpdyStreamRequest stream_request_; scoped_refptr stream_; scoped_refptr spdy_session_; diff --git a/net/spdy/spdy_proxy_client_socket_spdy2_unittest.cc b/net/spdy/spdy_proxy_client_socket_spdy2_unittest.cc index 22cf8ae..f089357 100644 --- a/net/spdy/spdy_proxy_client_socket_spdy2_unittest.cc +++ b/net/spdy/spdy_proxy_client_socket_spdy2_unittest.cc @@ -23,6 +23,7 @@ #include "net/spdy/spdy_http_utils.h" #include "net/spdy/spdy_protocol.h" #include "net/spdy/spdy_session_pool.h" +#include "net/spdy/spdy_test_util_common.h" #include "net/spdy/spdy_test_util_spdy2.h" #include "testing/platform_test.h" #include "testing/gtest/include/gtest/gtest.h" @@ -199,10 +200,9 @@ void SpdyProxyClientSocketSpdy2Test::Initialize(MockRead* reads, spdy_session_->InitializeWithSocket(connection.release(), false, OK); // Create the SPDY Stream. - ASSERT_EQ( - OK, - spdy_session_->CreateStream(url_, LOWEST, &spdy_stream_, net_log_.bound(), - CompletionCallback())); + spdy_stream_ = + CreateStreamSynchronously(spdy_session_, url_, LOWEST, net_log_.bound()); + ASSERT_TRUE(spdy_stream_.get() != NULL); // Create the SpdyProxyClientSocket. sock_.reset( diff --git a/net/spdy/spdy_proxy_client_socket_spdy3_unittest.cc b/net/spdy/spdy_proxy_client_socket_spdy3_unittest.cc index 05dc3fd..77a1502 100644 --- a/net/spdy/spdy_proxy_client_socket_spdy3_unittest.cc +++ b/net/spdy/spdy_proxy_client_socket_spdy3_unittest.cc @@ -23,6 +23,7 @@ #include "net/spdy/spdy_http_utils.h" #include "net/spdy/spdy_protocol.h" #include "net/spdy/spdy_session_pool.h" +#include "net/spdy/spdy_test_util_common.h" #include "net/spdy/spdy_test_util_spdy3.h" #include "testing/platform_test.h" #include "testing/gtest/include/gtest/gtest.h" @@ -199,10 +200,9 @@ void SpdyProxyClientSocketSpdy3Test::Initialize(MockRead* reads, spdy_session_->InitializeWithSocket(connection.release(), false, OK); // Create the SPDY Stream. - ASSERT_EQ( - OK, - spdy_session_->CreateStream(url_, LOWEST, &spdy_stream_, net_log_.bound(), - CompletionCallback())); + spdy_stream_ = + CreateStreamSynchronously(spdy_session_, url_, LOWEST, net_log_.bound()); + ASSERT_TRUE(spdy_stream_.get() != NULL); // Create the SpdyProxyClientSocket. sock_.reset( diff --git a/net/spdy/spdy_session.cc b/net/spdy/spdy_session.cc index 714b4f0..a2b5da4 100644 --- a/net/spdy/spdy_session.cc +++ b/net/spdy/spdy_session.cc @@ -4,6 +4,7 @@ #include "net/spdy/spdy_session.h" +#include #include #include "base/basictypes.h" @@ -201,6 +202,74 @@ const size_t kDefaultInitialRecvWindowSize = 10 * 1024 * 1024; // 10MB } // namespace +SpdyStreamRequest::SpdyStreamRequest() { + Reset(); +} + +SpdyStreamRequest::~SpdyStreamRequest() { + CancelRequest(); +} + +int SpdyStreamRequest::StartRequest( + const scoped_refptr& session, + const GURL& url, + RequestPriority priority, + const BoundNetLog& net_log, + const CompletionCallback& callback) { + DCHECK(session); + DCHECK(!session_); + DCHECK(!stream_); + DCHECK(callback_.is_null()); + + session_ = session; + url_ = url; + priority_ = priority; + net_log_ = net_log; + callback_ = callback; + + scoped_refptr stream; + int rv = session->TryCreateStream(this, &stream); + if (rv == OK) { + Reset(); + stream_ = stream; + } + return rv; +} + +void SpdyStreamRequest::CancelRequest() { + if (session_) + session_->CancelStreamRequest(this); + Reset(); +} + +scoped_refptr SpdyStreamRequest::ReleaseStream() { + DCHECK(!session_.get()); + scoped_refptr stream = stream_; + DCHECK(stream.get()); + Reset(); + return stream; +} + +void SpdyStreamRequest::OnRequestComplete( + const scoped_refptr& stream, + int rv) { + DCHECK(session_.get()); + DCHECK(!callback_.is_null()); + CompletionCallback callback = callback_; + Reset(); + stream_ = stream; + callback.Run(rv); +} + +void SpdyStreamRequest::Reset() { + session_ = NULL; + stream_ = NULL; + url_ = GURL(); + priority_ = MINIMUM_PRIORITY; + net_log_ = BoundNetLog(); + callback_.Reset(); +} + // static void SpdySession::SpdyIOBufferProducer::ActivateStream( SpdySession* spdy_session, @@ -302,28 +371,6 @@ SpdySession::SpdySession(const HostPortProxyPair& host_port_proxy_pair, // TODO(mbelshe): consider randomization of the stream_hi_water_mark. } -SpdySession::PendingCreateStream::PendingCreateStream( - const GURL& url, RequestPriority priority, - scoped_refptr* spdy_stream, - const BoundNetLog& stream_net_log, - const CompletionCallback& callback) - : url(&url), - priority(priority), - spdy_stream(spdy_stream), - stream_net_log(&stream_net_log), - callback(callback) { -} - -SpdySession::PendingCreateStream::~PendingCreateStream() {} - -SpdySession::CallbackResultPair::CallbackResultPair( - const CompletionCallback& callback_in, int result_in) - : callback(callback_in), - result(result_in) { -} - -SpdySession::CallbackResultPair::~CallbackResultPair() {} - SpdySession::~SpdySession() { if (state_ != STATE_CLOSED) { state_ = STATE_CLOSED; @@ -341,7 +388,10 @@ SpdySession::~SpdySession() { DCHECK_EQ(0u, num_active_streams()); DCHECK_EQ(0u, num_unclaimed_pushed_streams()); - DCHECK(pending_callback_map_.empty()); + for (int i = NUM_PRIORITIES - 1; i >= MINIMUM_PRIORITY; --i) { + DCHECK(pending_create_stream_queues_[i].empty()); + } + DCHECK(pending_stream_request_completions_.empty()); RecordHistograms(); @@ -460,95 +510,29 @@ int SpdySession::GetPushStream( return 0; } -int SpdySession::CreateStream( - const GURL& url, - RequestPriority priority, - scoped_refptr* spdy_stream, - const BoundNetLog& stream_net_log, - const CompletionCallback& callback) { +int SpdySession::TryCreateStream(SpdyStreamRequest* request, + scoped_refptr* stream) { if (!max_concurrent_streams_ || (active_streams_.size() + created_streams_.size() < max_concurrent_streams_)) { - return CreateStreamImpl(url, priority, spdy_stream, stream_net_log); + return CreateStream(*request, stream); } stalled_streams_++; net_log().AddEvent(NetLog::TYPE_SPDY_SESSION_STALLED_MAX_STREAMS); - pending_create_stream_queues_[priority].push( - PendingCreateStream(url, priority, spdy_stream, - stream_net_log, callback)); + pending_create_stream_queues_[request->priority()].push_back(request); return ERR_IO_PENDING; } -void SpdySession::ProcessPendingCreateStreams() { - while (!max_concurrent_streams_ || - (active_streams_.size() + created_streams_.size() < - max_concurrent_streams_)) { - bool no_pending_create_streams = true; - for (int i = NUM_PRIORITIES - 1; i >= MINIMUM_PRIORITY; --i) { - if (!pending_create_stream_queues_[i].empty()) { - PendingCreateStream pending_create = - pending_create_stream_queues_[i].front(); - pending_create_stream_queues_[i].pop(); - no_pending_create_streams = false; - int error = CreateStreamImpl(*pending_create.url, - pending_create.priority, - pending_create.spdy_stream, - *pending_create.stream_net_log); - scoped_refptr* stream = pending_create.spdy_stream; - DCHECK(!ContainsKey(pending_callback_map_, stream)); - pending_callback_map_.insert(std::make_pair(stream, - CallbackResultPair(pending_create.callback, error))); - MessageLoop::current()->PostTask( - FROM_HERE, - base::Bind(&SpdySession::InvokeUserStreamCreationCallback, - weak_factory_.GetWeakPtr(), stream)); - break; - } - } - if (no_pending_create_streams) - return; // there were no streams in any queue - } -} - -void SpdySession::CancelPendingCreateStreams( - const scoped_refptr* spdy_stream) { - PendingCallbackMap::iterator it = pending_callback_map_.find(spdy_stream); - if (it != pending_callback_map_.end()) { - pending_callback_map_.erase(it); - return; - } - - for (int i = 0; i < NUM_PRIORITIES; ++i) { - PendingCreateStreamQueue tmp; - // Make a copy removing this trans - while (!pending_create_stream_queues_[i].empty()) { - PendingCreateStream pending_create = - pending_create_stream_queues_[i].front(); - pending_create_stream_queues_[i].pop(); - if (pending_create.spdy_stream != spdy_stream) - tmp.push(pending_create); - } - // Now copy it back - while (!tmp.empty()) { - pending_create_stream_queues_[i].push(tmp.front()); - tmp.pop(); - } - } -} - -int SpdySession::CreateStreamImpl( - const GURL& url, - RequestPriority priority, - scoped_refptr* spdy_stream, - const BoundNetLog& stream_net_log) { - DCHECK_GE(priority, MINIMUM_PRIORITY); - DCHECK_LT(priority, NUM_PRIORITIES); +int SpdySession::CreateStream(const SpdyStreamRequest& request, + scoped_refptr* stream) { + DCHECK_GE(request.priority(), MINIMUM_PRIORITY); + DCHECK_LT(request.priority(), NUM_PRIORITIES); // Make sure that we don't try to send https/wss over an unauthenticated, but // encrypted SSL socket. if (is_secure_ && certificate_error_code_ != OK && - (url.SchemeIs("https") || url.SchemeIs("wss"))) { + (request.url().SchemeIs("https") || request.url().SchemeIs("wss"))) { RecordProtocolErrorHistogram( PROTOCOL_ERROR_REQUEST_FOR_SECURE_CONTENT_OVER_INSECURE_SESSION); CloseSessionOnError( @@ -559,27 +543,81 @@ int SpdySession::CreateStreamImpl( return ERR_SPDY_PROTOCOL_ERROR; } - const std::string& path = url.PathForRequest(); + const std::string& path = request.url().PathForRequest(); - *spdy_stream = new SpdyStream(this, - false, - stream_net_log); - const scoped_refptr& stream = *spdy_stream; + *stream = new SpdyStream(this, false, request.net_log()); - stream->set_priority(priority); - stream->set_path(path); - stream->set_send_window_size(stream_initial_send_window_size_); - stream->set_recv_window_size(stream_initial_recv_window_size_); - created_streams_.insert(stream); + (*stream)->set_priority(request.priority()); + (*stream)->set_path(path); + (*stream)->set_send_window_size(stream_initial_send_window_size_); + (*stream)->set_recv_window_size(stream_initial_recv_window_size_); + created_streams_.insert(*stream); - UMA_HISTOGRAM_CUSTOM_COUNTS("Net.SpdyPriorityCount", - static_cast(priority), 0, 10, 11); + UMA_HISTOGRAM_CUSTOM_COUNTS( + "Net.SpdyPriorityCount", + static_cast(request.priority()), 0, 10, 11); // TODO(mbelshe): Optimize memory allocations return OK; } +void SpdySession::CancelStreamRequest(SpdyStreamRequest* request) { + if (DCHECK_IS_ON()) { + // |request| should not be in a queue not matching its priority. + for (int i = 0; i < NUM_PRIORITIES; ++i) { + if (request->priority() == i) + continue; + PendingStreamRequestQueue* queue = &pending_create_stream_queues_[i]; + DCHECK(std::find(queue->begin(), queue->end(), request) == queue->end()); + } + } + + PendingStreamRequestQueue* queue = + &pending_create_stream_queues_[request->priority()]; + // Remove |request| from |queue| while preserving the order of the + // other elements. + PendingStreamRequestQueue::iterator it = + std::find(queue->begin(), queue->end(), request); + if (it != queue->end()) { + it = queue->erase(it); + // |request| should be in the queue at most once, and if it is + // present, should not be pending completion. + DCHECK(std::find(it, queue->end(), request) == queue->end()); + DCHECK(!ContainsKey(pending_stream_request_completions_, + request)); + return; + } + + pending_stream_request_completions_.erase(request); +} + +void SpdySession::ProcessPendingStreamRequests() { + while (!max_concurrent_streams_ || + (active_streams_.size() + created_streams_.size() < + max_concurrent_streams_)) { + bool no_pending_create_streams = true; + for (int i = NUM_PRIORITIES - 1; i >= MINIMUM_PRIORITY; --i) { + if (!pending_create_stream_queues_[i].empty()) { + SpdyStreamRequest* pending_request = + pending_create_stream_queues_[i].front(); + pending_create_stream_queues_[i].pop_front(); + no_pending_create_streams = false; + DCHECK(!ContainsKey(pending_stream_request_completions_, + pending_request)); + pending_stream_request_completions_.insert(pending_request); + MessageLoop::current()->PostTask( + FROM_HERE, + base::Bind(&SpdySession::CompleteStreamRequest, + weak_factory_.GetWeakPtr(), pending_request)); + break; + } + } + if (no_pending_create_streams) + return; // there were no streams in any queue + } +} + bool SpdySession::NeedsCredentials() const { if (!is_secure_) return false; @@ -778,7 +816,7 @@ void SpdySession::CloseStream(SpdyStreamId stream_id, int status) { void SpdySession::CloseCreatedStream(SpdyStream* stream, int status) { DCHECK_EQ(0u, stream->stream_id()); created_streams_.erase(scoped_refptr(stream)); - ProcessPendingCreateStreams(); + ProcessPendingStreamRequests(); } void SpdySession::ResetStream(SpdyStreamId stream_id, @@ -1053,11 +1091,11 @@ void SpdySession::CloseAllStreams(net::Error status) { } for (int i = 0; i < NUM_PRIORITIES; ++i) { - while (!pending_create_stream_queues_[i].empty()) { - PendingCreateStream pending_create = - pending_create_stream_queues_[i].front(); - pending_create_stream_queues_[i].pop(); - pending_create.callback.Run(ERR_ABORTED); + PendingStreamRequestQueue queue; + queue.swap(pending_create_stream_queues_[i]); + for (PendingStreamRequestQueue::const_iterator it = queue.begin(); + it != queue.end(); ++it) { + (*it)->OnRequestComplete(NULL, ERR_ABORTED); } } @@ -1274,7 +1312,7 @@ void SpdySession::DeleteStream(SpdyStreamId id, int status) { active_streams_.erase(it2); if (stream) stream->OnClose(status); - ProcessPendingCreateStreams(); + ProcessPendingStreamRequests(); } void SpdySession::RemoveFromPool() { @@ -1861,7 +1899,7 @@ void SpdySession::HandleSetting(uint32 id, uint32 value) { case SETTINGS_MAX_CONCURRENT_STREAMS: max_concurrent_streams_ = std::min(static_cast(value), kMaxConcurrentStreamLimit); - ProcessPendingCreateStreams(); + ProcessPendingStreamRequests(); break; case SETTINGS_INITIAL_WINDOW_SIZE: { if (flow_control_state_ < FLOW_CONTROL_STREAM) { @@ -2087,18 +2125,19 @@ void SpdySession::RecordHistograms() { } } -void SpdySession::InvokeUserStreamCreationCallback( - scoped_refptr* stream) { - PendingCallbackMap::iterator it = pending_callback_map_.find(stream); +void SpdySession::CompleteStreamRequest(SpdyStreamRequest* pending_request) { + PendingStreamRequestCompletionSet::iterator it = + pending_stream_request_completions_.find(pending_request); - // Exit if the request has already been cancelled. - if (it == pending_callback_map_.end()) + // Abort if the request has already been cancelled. + if (it == pending_stream_request_completions_.end()) return; - CompletionCallback callback = it->second.callback; - int result = it->second.result; - pending_callback_map_.erase(it); - callback.Run(result); + scoped_refptr stream; + int rv = CreateStream(*pending_request, &stream); + pending_stream_request_completions_.erase(it); + + pending_request->OnRequestComplete(stream, rv); } SSLClientSocket* SpdySession::GetSSLClientSocket() const { diff --git a/net/spdy/spdy_session.h b/net/spdy/spdy_session.h index 865589d..b025bdf 100644 --- a/net/spdy/spdy_session.h +++ b/net/spdy/spdy_session.h @@ -5,10 +5,10 @@ #ifndef NET_SPDY_SPDY_SESSION_H_ #define NET_SPDY_SPDY_SESSION_H_ -#include +#include #include #include -#include +#include #include #include "base/gtest_prod_util.h" @@ -98,6 +98,66 @@ COMPILE_ASSERT(PROTOCOL_ERROR_UNEXPECTED_PING == RST_STREAM_NUM_STATUS_CODES + STATUS_CODE_INVALID), SpdyProtocolErrorDetails_SpdyErrors_mismatch); +// A helper class used to manage a request to create a stream. +class NET_EXPORT_PRIVATE SpdyStreamRequest { + public: + SpdyStreamRequest(); + // Calls CancelRequest(). + ~SpdyStreamRequest(); + + // Starts the request to create a stream. If OK is returned, then + // ReleaseStream() may be called. If ERR_IO_PENDING is returned, + // then when the stream is created, |callback| will be called, at + // which point ReleaseStream() may be called. Otherwise, the stream + // is not created, an error is returned, and ReleaseStream() may not + // be called. + // + // If OK is returned, must not be called again without + // ReleaseStream() being called first. If ERR_IO_PENDING is + // returned, must not be called again without CancelRequest() or + // ReleaseStream() being called first. Otherwise, in case of an + // immediate error, this may be called again. + int StartRequest(const scoped_refptr& session, + const GURL& url, + RequestPriority priority, + const BoundNetLog& net_log, + const CompletionCallback& callback); + + // Cancels any pending stream creation request. May be called + // repeatedly. + void CancelRequest(); + + // Transfers the created stream (guaranteed to not be NULL) to the + // caller. Must be called at most once after StartRequest() returns + // OK or |callback| is called with OK. + scoped_refptr ReleaseStream(); + + private: + friend class SpdySession; + + // Called by |session_| when the stream attempt is + // finished. |stream| is non-NULL exactly when |rv| is OK. Also + // called with a NULL stream and ERR_ABORTED if |session_| is + // destroyed while the stream attempt is still pending. + void OnRequestComplete(const scoped_refptr& stream, int rv); + + // Accessors called by |session_|. + const GURL& url() const { return url_; } + RequestPriority priority() const { return priority_; } + const BoundNetLog& net_log() const { return net_log_; } + + void Reset(); + + scoped_refptr session_; + scoped_refptr stream_; + GURL url_; + RequestPriority priority_; + BoundNetLog net_log_; + CompletionCallback callback_; + + DISALLOW_COPY_AND_ASSIGN(SpdyStreamRequest); +}; + class NET_EXPORT SpdySession : public base::RefCounted, public BufferedSpdyFramerVisitorInterface { public: @@ -177,18 +237,6 @@ class NET_EXPORT SpdySession : public base::RefCounted, scoped_refptr* spdy_stream, const BoundNetLog& stream_net_log); - // Create a new stream for a given |url|. Writes it out to |spdy_stream|. - // Returns a net error code, possibly ERR_IO_PENDING. - int CreateStream( - const GURL& url, - RequestPriority priority, - scoped_refptr* spdy_stream, - const BoundNetLog& stream_net_log, - const CompletionCallback& callback); - - // Remove PendingCreateStream objects on transaction deletion - void CancelPendingCreateStreams(const scoped_refptr* spdy_stream); - // Used by SpdySessionPool to initialize with a pre-existing SSL socket. For // testing, setting is_secure to false allows initialization with a // pre-existing TCP socket. @@ -384,6 +432,7 @@ class NET_EXPORT SpdySession : public base::RefCounted, private: friend class base::RefCounted; + friend class SpdyStreamRequest; // Allow tests to access our innards for testing purposes. FRIEND_TEST_ALL_PREFIXES(SpdySessionSpdy2Test, ClientPing); @@ -402,22 +451,9 @@ class NET_EXPORT SpdySession : public base::RefCounted, FRIEND_TEST_ALL_PREFIXES(SpdySessionSpdy3Test, AdjustSendWindowSize31); FRIEND_TEST_ALL_PREFIXES(SpdySessionSpdy3Test, SessionFlowControlEndToEnd31); - struct PendingCreateStream { - PendingCreateStream(const GURL& url, RequestPriority priority, - scoped_refptr* spdy_stream, - const BoundNetLog& stream_net_log, - const CompletionCallback& callback); + typedef std::deque PendingStreamRequestQueue; + typedef std::set PendingStreamRequestCompletionSet; - ~PendingCreateStream(); - - const GURL* url; - RequestPriority priority; - scoped_refptr* spdy_stream; - const BoundNetLog* stream_net_log; - CompletionCallback callback; - }; - typedef std::queue > - PendingCreateStreamQueue; typedef std::map > ActiveStreamMap; typedef std::map, base::TimeTicks> > PushedStreamMap; @@ -425,6 +461,7 @@ class NET_EXPORT SpdySession : public base::RefCounted, typedef std::set > CreatedStreamSet; typedef std::map StreamProducerMap; + class SpdyIOBufferProducerCompare { public: bool operator() (const SpdyIOBufferProducer* lhs, @@ -432,21 +469,11 @@ class NET_EXPORT SpdySession : public base::RefCounted, return lhs->GetPriority() < rhs->GetPriority(); } }; + typedef std::priority_queue, SpdyIOBufferProducerCompare> WriteQueue; - struct CallbackResultPair { - CallbackResultPair(const CompletionCallback& callback_in, int result_in); - ~CallbackResultPair(); - - CompletionCallback callback; - int result; - }; - - typedef std::map*, CallbackResultPair> - PendingCallbackMap; - enum State { STATE_IDLE, STATE_CONNECTING, @@ -457,12 +484,28 @@ class NET_EXPORT SpdySession : public base::RefCounted, virtual ~SpdySession(); - void ProcessPendingCreateStreams(); - int CreateStreamImpl( - const GURL& url, - RequestPriority priority, - scoped_refptr* spdy_stream, - const BoundNetLog& stream_net_log); + // Called by SpdyStreamRequest to start a request to create a + // stream. If OK is returned, then |stream| will be filled in with a + // valid stream. If ERR_IO_PENDING is returned, then + // |request->OnRequestComplete()| will be called when the stream is + // created (unless it is cancelled). Otherwise, no stream is created + // and the error is returned. + int TryCreateStream(SpdyStreamRequest* request, + scoped_refptr* stream); + + // Actually create a stream into |stream|. Returns OK if successful; + // otherwise, returns an error and |stream| is not filled. + int CreateStream(const SpdyStreamRequest& request, + scoped_refptr* stream); + + // Called by SpdyStreamRequest to remove |request| from the stream + // creation queue. + void CancelStreamRequest(SpdyStreamRequest* request); + + // Called when there is room to create more streams (e.g., a stream + // was closed). Processes as many pending stream requests as + // possible. + void ProcessPendingStreamRequests(); // Start the DoLoop to read data from socket. void StartRead(); @@ -493,7 +536,7 @@ class NET_EXPORT SpdySession : public base::RefCounted, // SETTINGS control frame, update our SpdySession accordingly. void HandleSetting(uint32 id, uint32 value); - // Adjust the send window size of all ActiveStreams and PendingCreateStreams. + // Adjust the send window size of all ActiveStreams and PendingStreamRequests. void UpdateStreamsSendWindowSize(int32 delta_window_size); // Send the PING (preface-PING) frame. @@ -558,7 +601,7 @@ class NET_EXPORT SpdySession : public base::RefCounted, // Invokes a user callback for stream creation. We provide this method so it // can be deferred to the MessageLoop, so we avoid re-entrancy problems. - void InvokeUserStreamCreationCallback(scoped_refptr* stream); + void CompleteStreamRequest(SpdyStreamRequest* pending_request); // Remove old unclaimed pushed streams. void DeleteExpiredPushedStreams(); @@ -651,11 +694,6 @@ class NET_EXPORT SpdySession : public base::RefCounted, // method. base::WeakPtrFactory weak_factory_; - // Map of the SpdyStreams for which we have a pending Task to invoke a - // callback. This is necessary since, before we invoke said callback, it's - // possible that the request is cancelled. - PendingCallbackMap pending_callback_map_; - // The domain this session is connected to. const HostPortProxyPair host_port_proxy_pair_; @@ -676,9 +714,15 @@ class NET_EXPORT SpdySession : public base::RefCounted, int stream_hi_water_mark_; // The next stream id to use. - // Queue, for each priority, of pending Create Streams that have not - // yet been satisfied - PendingCreateStreamQueue pending_create_stream_queues_[NUM_PRIORITIES]; + // Queue, for each priority, of pending stream requests that have + // not yet been satisfied. + PendingStreamRequestQueue pending_create_stream_queues_[NUM_PRIORITIES]; + + // A set of requests that are waiting to be completed (i.e., for the + // stream to actually be created). This is necessary since we kick + // off the stream creation asynchronously, and so the request may be + // cancelled before the asynchronous task to create the stream runs. + PendingStreamRequestCompletionSet pending_stream_request_completions_; // Map from stream id to all active streams. Streams are active in the sense // that they have a consumer (typically SpdyNetworkTransaction and regardless diff --git a/net/spdy/spdy_session_spdy2_unittest.cc b/net/spdy/spdy_session_spdy2_unittest.cc index 93263b3..65cc61f 100644 --- a/net/spdy/spdy_session_spdy2_unittest.cc +++ b/net/spdy/spdy_session_spdy2_unittest.cc @@ -17,6 +17,7 @@ #include "net/spdy/spdy_session_pool.h" #include "net/spdy/spdy_session_test_util.h" #include "net/spdy/spdy_stream.h" +#include "net/spdy/spdy_test_util_common.h" #include "net/spdy/spdy_test_util_spdy2.h" #include "testing/platform_test.h" @@ -272,11 +273,10 @@ TEST_F(SpdySessionSpdy2Test, ClientPing) { scoped_refptr session = CreateInitializedSession(); - scoped_refptr spdy_stream1; + scoped_refptr spdy_stream1 = + CreateStreamSynchronously(session, test_url_, MEDIUM, BoundNetLog()); + ASSERT_TRUE(spdy_stream1.get() != NULL); TestCompletionCallback callback1; - EXPECT_EQ(OK, session->CreateStream(test_url_, MEDIUM, &spdy_stream1, - BoundNetLog(), callback1.callback())); - scoped_ptr delegate( new TestSpdyStreamDelegate(callback1.callback())); spdy_stream1->SetDelegate(delegate.get()); @@ -328,10 +328,10 @@ TEST_F(SpdySessionSpdy2Test, ServerPing) { scoped_refptr session = CreateInitializedSession(); - scoped_refptr spdy_stream1; + scoped_refptr spdy_stream1 = + CreateStreamSynchronously(session, test_url_, MEDIUM, BoundNetLog()); + ASSERT_TRUE(spdy_stream1.get() != NULL); TestCompletionCallback callback1; - EXPECT_EQ(OK, session->CreateStream(test_url_, MEDIUM, &spdy_stream1, - BoundNetLog(), callback1.callback())); scoped_ptr delegate( new TestSpdyStreamDelegate(callback1.callback())); spdy_stream1->SetDelegate(delegate.get()); @@ -420,10 +420,10 @@ TEST_F(SpdySessionSpdy2Test, FailedPing) { scoped_refptr session = CreateInitializedSession(); - scoped_refptr spdy_stream1; + scoped_refptr spdy_stream1 = + CreateStreamSynchronously(session, test_url_, MEDIUM, BoundNetLog()); + ASSERT_TRUE(spdy_stream1.get() != NULL); TestCompletionCallback callback1; - EXPECT_EQ(OK, session->CreateStream(test_url_, MEDIUM, &spdy_stream1, - BoundNetLog(), callback1.callback())); scoped_ptr delegate( new TestSpdyStreamDelegate(callback1.callback())); spdy_stream1->SetDelegate(delegate.get()); @@ -457,40 +457,6 @@ TEST_F(SpdySessionSpdy2Test, FailedPing) { session = NULL; } -class StreamReleaserCallback : public TestCompletionCallbackBase { - public: - StreamReleaserCallback(SpdySession* session, - SpdyStream* first_stream) - : session_(session), - first_stream_(first_stream), - ALLOW_THIS_IN_INITIALIZER_LIST(callback_( - base::Bind(&StreamReleaserCallback::OnComplete, - base::Unretained(this)))) { - } - - virtual ~StreamReleaserCallback() {} - - scoped_refptr* stream() { return &stream_; } - - const CompletionCallback& callback() const { return callback_; } - - private: - void OnComplete(int result) { - session_->CloseSessionOnError(ERR_FAILED, false, "On complete."); - session_ = NULL; - first_stream_->Cancel(); - first_stream_ = NULL; - stream_->Cancel(); - stream_ = NULL; - SetResult(result); - } - - scoped_refptr session_; - scoped_refptr first_stream_; - scoped_refptr stream_; - CompletionCallback callback_; -}; - // TODO(kristianm): Could also test with more sessions where some are idle, // and more than one session to a HostPortPair. TEST_F(SpdySessionSpdy2Test, CloseIdleSessions) { @@ -501,33 +467,30 @@ TEST_F(SpdySessionSpdy2Test, CloseIdleSessions) { HostPortPair test_host_port_pair1(kTestHost1, 80); HostPortProxyPair pair1(test_host_port_pair1, ProxyServer::Direct()); scoped_refptr session1 = GetSession(pair1); - scoped_refptr spdy_stream1; - TestCompletionCallback callback1; GURL url1(kTestHost1); - EXPECT_EQ(OK, session1->CreateStream(url1, MEDIUM, &spdy_stream1, - BoundNetLog(), callback1.callback())); + scoped_refptr spdy_stream1 = + CreateStreamSynchronously(session1, url1, MEDIUM, BoundNetLog()); + ASSERT_TRUE(spdy_stream1.get() != NULL); // Set up session 2 const std::string kTestHost2("http://www.b.com"); HostPortPair test_host_port_pair2(kTestHost2, 80); HostPortProxyPair pair2(test_host_port_pair2, ProxyServer::Direct()); scoped_refptr session2 = GetSession(pair2); - scoped_refptr spdy_stream2; - TestCompletionCallback callback2; GURL url2(kTestHost2); - EXPECT_EQ(OK, session2->CreateStream(url2, MEDIUM, &spdy_stream2, - BoundNetLog(), callback2.callback())); + scoped_refptr spdy_stream2 = + CreateStreamSynchronously(session2, url2, MEDIUM, BoundNetLog()); + ASSERT_TRUE(spdy_stream2.get() != NULL); // Set up session 3 const std::string kTestHost3("http://www.c.com"); HostPortPair test_host_port_pair3(kTestHost3, 80); HostPortProxyPair pair3(test_host_port_pair3, ProxyServer::Direct()); scoped_refptr session3 = GetSession(pair3); - scoped_refptr spdy_stream3; - TestCompletionCallback callback3; GURL url3(kTestHost3); - EXPECT_EQ(OK, session3->CreateStream(url3, MEDIUM, &spdy_stream3, - BoundNetLog(), callback3.callback())); + scoped_refptr spdy_stream3 = + CreateStreamSynchronously(session3, url3, MEDIUM, BoundNetLog()); + ASSERT_TRUE(spdy_stream3.get() != NULL); // All sessions are active and not closed EXPECT_TRUE(session1->is_active()); @@ -624,17 +587,17 @@ TEST_F(SpdySessionSpdy2Test, OnSettings) { scoped_refptr session = CreateInitializedSession(); // Create 2 streams. First will succeed. Second will be pending. - scoped_refptr spdy_stream1; - TestCompletionCallback callback1; - EXPECT_EQ(OK, - session->CreateStream(test_url_, MEDIUM, &spdy_stream1, - BoundNetLog(), callback1.callback())); + scoped_refptr spdy_stream1 = + CreateStreamSynchronously(session, test_url_, MEDIUM, BoundNetLog()); + ASSERT_TRUE(spdy_stream1.get() != NULL); StreamReleaserCallback stream_releaser(session, spdy_stream1); + SpdyStreamRequest request; ASSERT_EQ(ERR_IO_PENDING, - session->CreateStream(test_url_, MEDIUM, stream_releaser.stream(), - BoundNetLog(), stream_releaser.callback())); + request.StartRequest(session, test_url_, MEDIUM, + BoundNetLog(), + stream_releaser.MakeCallback(&request))); // Make sure |stream_releaser| holds the last refs. session = NULL; @@ -674,25 +637,26 @@ TEST_F(SpdySessionSpdy2Test, CancelPendingCreateStream) { scoped_refptr session = CreateInitializedSession(); + // Create 2 streams. First will succeed. Second will be pending. + scoped_refptr spdy_stream1 = + CreateStreamSynchronously(session, test_url_, MEDIUM, BoundNetLog()); + ASSERT_TRUE(spdy_stream1.get() != NULL); + // Use scoped_ptr to let us invalidate the memory when we want to, to trigger // a valgrind error if the callback is invoked when it's not supposed to be. scoped_ptr callback(new TestCompletionCallback); - // Create 2 streams. First will succeed. Second will be pending. - scoped_refptr spdy_stream1; - ASSERT_EQ(OK, session->CreateStream(test_url_, MEDIUM, &spdy_stream1, - BoundNetLog(), callback->callback())); - - scoped_refptr spdy_stream2; + SpdyStreamRequest request; ASSERT_EQ(ERR_IO_PENDING, - session->CreateStream(test_url_, MEDIUM, &spdy_stream2, - BoundNetLog(), callback->callback())); + request.StartRequest(session, test_url_, MEDIUM, + BoundNetLog(), + callback->callback())); // Release the first one, this will allow the second to be created. spdy_stream1->Cancel(); spdy_stream1 = NULL; - session->CancelPendingCreateStreams(&spdy_stream2); + request.CancelRequest(); callback.reset(); // Should not crash when running the pending callback. @@ -1028,17 +992,16 @@ TEST_F(SpdySessionSpdy2Test, OutOfOrderSynStreams) { scoped_refptr session = CreateInitializedSession(); - scoped_refptr spdy_stream1; - TestCompletionCallback callback1; GURL url("http://www.google.com"); - EXPECT_EQ(OK, session->CreateStream(url, LOWEST, &spdy_stream1, - BoundNetLog(), callback1.callback())); + + scoped_refptr spdy_stream1 = + CreateStreamSynchronously(session, url, LOWEST, BoundNetLog()); + ASSERT_TRUE(spdy_stream1.get() != NULL); EXPECT_EQ(0u, spdy_stream1->stream_id()); - scoped_refptr spdy_stream2; - TestCompletionCallback callback2; - EXPECT_EQ(OK, session->CreateStream(url, HIGHEST, &spdy_stream2, - BoundNetLog(), callback2.callback())); + scoped_refptr spdy_stream2 = + CreateStreamSynchronously(session, url, HIGHEST, BoundNetLog()); + ASSERT_TRUE(spdy_stream2.get() != NULL); EXPECT_EQ(0u, spdy_stream2->stream_id()); scoped_ptr headers(new SpdyHeaderBlock); @@ -1101,18 +1064,16 @@ TEST_F(SpdySessionSpdy2Test, CancelStream) { scoped_refptr session = CreateInitializedSession(); - scoped_refptr spdy_stream1; - TestCompletionCallback callback1; GURL url1("http://www.google.com"); - EXPECT_EQ(OK, session->CreateStream(url1, HIGHEST, &spdy_stream1, - BoundNetLog(), callback1.callback())); + scoped_refptr spdy_stream1 = + CreateStreamSynchronously(session, url1, HIGHEST, BoundNetLog()); + ASSERT_TRUE(spdy_stream1.get() != NULL); EXPECT_EQ(0u, spdy_stream1->stream_id()); - scoped_refptr spdy_stream2; - TestCompletionCallback callback2; GURL url2("http://www.google.com"); - EXPECT_EQ(OK, session->CreateStream(url2, LOWEST, &spdy_stream2, - BoundNetLog(), callback2.callback())); + scoped_refptr spdy_stream2 = + CreateStreamSynchronously(session, url2, LOWEST, BoundNetLog()); + ASSERT_TRUE(spdy_stream2.get() != NULL); EXPECT_EQ(0u, spdy_stream2->stream_id()); scoped_ptr headers(new SpdyHeaderBlock); @@ -1176,18 +1137,16 @@ TEST_F(SpdySessionSpdy2Test, CloseSessionWithTwoCreatedStreams) { scoped_refptr session = CreateInitializedSession(); - scoped_refptr spdy_stream1; - TestCompletionCallback callback1; GURL url1("http://www.google.com"); - EXPECT_EQ(OK, session->CreateStream(url1, HIGHEST, &spdy_stream1, - BoundNetLog(), callback1.callback())); + scoped_refptr spdy_stream1 = + CreateStreamSynchronously(session, url1, HIGHEST, BoundNetLog()); + ASSERT_TRUE(spdy_stream1.get() != NULL); EXPECT_EQ(0u, spdy_stream1->stream_id()); - scoped_refptr spdy_stream2; - TestCompletionCallback callback2; GURL url2("http://www.google.com"); - EXPECT_EQ(OK, session->CreateStream(url2, LOWEST, &spdy_stream2, - BoundNetLog(), callback2.callback())); + scoped_refptr spdy_stream2 = + CreateStreamSynchronously(session, url2, LOWEST, BoundNetLog()); + ASSERT_TRUE(spdy_stream2.get() != NULL); EXPECT_EQ(0u, spdy_stream2->stream_id()); scoped_ptr headers(new SpdyHeaderBlock); @@ -1422,27 +1381,27 @@ TEST_F(SpdySessionSpdy2Test, CloseTwoStalledCreateStream) { // Read the settings frame. data.RunFor(1); - scoped_refptr spdy_stream1; - TestCompletionCallback callback1; GURL url1("http://www.google.com"); - EXPECT_EQ(OK, session->CreateStream(url1, LOWEST, &spdy_stream1, - BoundNetLog(), callback1.callback())); - + scoped_refptr spdy_stream1 = + CreateStreamSynchronously(session, url1, LOWEST, BoundNetLog()); + ASSERT_TRUE(spdy_stream1.get() != NULL); EXPECT_EQ(0u, spdy_stream1->stream_id()); - scoped_refptr spdy_stream2; TestCompletionCallback callback2; GURL url2("http://www.google.com"); - EXPECT_EQ(ERR_IO_PENDING, - session->CreateStream(url2, LOWEST, &spdy_stream2, - BoundNetLog(), callback2.callback())); + SpdyStreamRequest request2; + ASSERT_EQ(ERR_IO_PENDING, + request2.StartRequest(session, url2, LOWEST, + BoundNetLog(), + callback2.callback())); - scoped_refptr spdy_stream3; TestCompletionCallback callback3; GURL url3("http://www.google.com"); - EXPECT_EQ(ERR_IO_PENDING, - session->CreateStream(url3, LOWEST, &spdy_stream3, - BoundNetLog(), callback3.callback())); + SpdyStreamRequest request3; + ASSERT_EQ(ERR_IO_PENDING, + request3.StartRequest(session, url3, LOWEST, + BoundNetLog(), + callback3.callback())); EXPECT_EQ(1u, session->num_active_streams() + session->num_created_streams()); EXPECT_EQ(2u, session->pending_create_stream_queues(LOWEST)); @@ -1462,33 +1421,34 @@ TEST_F(SpdySessionSpdy2Test, CloseTwoStalledCreateStream) { EXPECT_TRUE(spdy_stream1->HasUrl()); spdy_stream1->SendRequest(false); - // Run until 1st stream is closed. + // Run until 1st stream is closed and 2nd one is opened. EXPECT_EQ(0u, spdy_stream1->stream_id()); data.RunFor(3); EXPECT_EQ(1u, spdy_stream1->stream_id()); - EXPECT_EQ(1u, session->num_active_streams() + session->num_created_streams()); - EXPECT_EQ(1u, session->pending_create_stream_queues(LOWEST)); + EXPECT_EQ(2u, session->num_active_streams() + session->num_created_streams()); + EXPECT_EQ(0u, session->pending_create_stream_queues(LOWEST)); - EXPECT_TRUE(spdy_stream2.get() != NULL); - spdy_stream2->set_spdy_headers(headers2.Pass()); - EXPECT_TRUE(spdy_stream2->HasUrl()); - spdy_stream2->SendRequest(false); + scoped_refptr stream2 = request2.ReleaseStream(); + stream2->set_spdy_headers(headers2.Pass()); + EXPECT_TRUE(stream2->HasUrl()); + stream2->SendRequest(false); // Run until 2nd stream is closed. - EXPECT_EQ(0u, spdy_stream2->stream_id()); + EXPECT_EQ(0u, stream2->stream_id()); data.RunFor(3); - EXPECT_EQ(3u, spdy_stream2->stream_id()); + EXPECT_EQ(3u, stream2->stream_id()); EXPECT_EQ(1u, session->num_active_streams() + session->num_created_streams()); EXPECT_EQ(0u, session->pending_create_stream_queues(LOWEST)); - EXPECT_TRUE(spdy_stream3.get() != NULL); - spdy_stream3->set_spdy_headers(headers3.Pass()); - EXPECT_TRUE(spdy_stream3->HasUrl()); - spdy_stream3->SendRequest(false); + scoped_refptr stream3 = request3.ReleaseStream(); + ASSERT_TRUE(stream3.get() != NULL); + stream3->set_spdy_headers(headers3.Pass()); + EXPECT_TRUE(stream3->HasUrl()); + stream3->SendRequest(false); - EXPECT_EQ(0u, spdy_stream3->stream_id()); + EXPECT_EQ(0u, stream3->stream_id()); data.RunFor(4); - EXPECT_EQ(5u, spdy_stream3->stream_id()); + EXPECT_EQ(5u, stream3->stream_id()); EXPECT_EQ(0u, session->num_active_streams() + session->num_created_streams()); EXPECT_EQ(0u, session->pending_create_stream_queues(LOWEST)); } @@ -1520,27 +1480,27 @@ TEST_F(SpdySessionSpdy2Test, CancelTwoStalledCreateStream) { scoped_refptr session = CreateInitializedSession(); - scoped_refptr spdy_stream1; - TestCompletionCallback callback1; GURL url1("http://www.google.com"); - ASSERT_EQ(OK, - session->CreateStream(url1, LOWEST, &spdy_stream1, - BoundNetLog(), callback1.callback())); + scoped_refptr spdy_stream1 = + CreateStreamSynchronously(session, url1, LOWEST, BoundNetLog()); + ASSERT_TRUE(spdy_stream1.get() != NULL); EXPECT_EQ(0u, spdy_stream1->stream_id()); - scoped_refptr spdy_stream2; TestCompletionCallback callback2; GURL url2("http://www.google.com"); + SpdyStreamRequest request2; ASSERT_EQ(ERR_IO_PENDING, - session->CreateStream(url2, LOWEST, &spdy_stream2, - BoundNetLog(), callback2.callback())); + request2.StartRequest(session, url2, LOWEST, + BoundNetLog(), + callback2.callback())); - scoped_refptr spdy_stream3; TestCompletionCallback callback3; GURL url3("http://www.google.com"); + SpdyStreamRequest request3; ASSERT_EQ(ERR_IO_PENDING, - session->CreateStream(url3, LOWEST, &spdy_stream3, - BoundNetLog(), callback3.callback())); + request3.StartRequest(session, url3, LOWEST, + BoundNetLog(), + callback3.callback())); EXPECT_EQ(1u, session->num_active_streams() + session->num_created_streams()); EXPECT_EQ(2u, session->pending_create_stream_queues(LOWEST)); @@ -1549,23 +1509,18 @@ TEST_F(SpdySessionSpdy2Test, CancelTwoStalledCreateStream) { EXPECT_TRUE(spdy_stream1.get() != NULL); spdy_stream1->Cancel(); spdy_stream1 = NULL; - session->CancelPendingCreateStreams(&spdy_stream1); - EXPECT_EQ(1u, session->num_active_streams() + session->num_created_streams()); - EXPECT_EQ(1u, session->pending_create_stream_queues(LOWEST)); + + callback2.WaitForResult(); + EXPECT_EQ(2u, session->num_active_streams() + session->num_created_streams()); + EXPECT_EQ(0u, session->pending_create_stream_queues(LOWEST)); // Cancel the second stream, this will allow the third stream to be created. - EXPECT_TRUE(spdy_stream2.get() != NULL); - spdy_stream2->Cancel(); - spdy_stream2 = NULL; - session->CancelPendingCreateStreams(&spdy_stream2); + request2.ReleaseStream()->Cancel(); EXPECT_EQ(1u, session->num_active_streams() + session->num_created_streams()); EXPECT_EQ(0u, session->pending_create_stream_queues(LOWEST)); // Cancel the third stream. - EXPECT_TRUE(spdy_stream3.get() != NULL); - spdy_stream3->Cancel(); - spdy_stream3 = NULL; - session->CancelPendingCreateStreams(&spdy_stream3); + request3.ReleaseStream()->Cancel(); EXPECT_EQ(0u, session->num_active_streams() + session->num_created_streams()); EXPECT_EQ(0u, session->pending_create_stream_queues(LOWEST)); } @@ -1682,11 +1637,10 @@ TEST_F(SpdySessionSpdy2Test, ReadDataWithoutYielding) { scoped_refptr session = CreateInitializedSession(); - scoped_refptr spdy_stream1; - TestCompletionCallback callback1; GURL url1("http://www.google.com"); - EXPECT_EQ(OK, session->CreateStream(url1, MEDIUM, &spdy_stream1, - BoundNetLog(), callback1.callback())); + scoped_refptr spdy_stream1 = + CreateStreamSynchronously(session, url1, MEDIUM, BoundNetLog()); + ASSERT_TRUE(spdy_stream1.get() != NULL); EXPECT_EQ(0u, spdy_stream1->stream_id()); scoped_ptr headers(new SpdyHeaderBlock); @@ -1773,11 +1727,10 @@ TEST_F(SpdySessionSpdy2Test, TestYieldingDuringReadData) { scoped_refptr session = CreateInitializedSession(); - scoped_refptr spdy_stream1; - TestCompletionCallback callback1; GURL url1("http://www.google.com"); - EXPECT_EQ(OK, session->CreateStream(url1, MEDIUM, &spdy_stream1, - BoundNetLog(), callback1.callback())); + scoped_refptr spdy_stream1 = + CreateStreamSynchronously(session, url1, MEDIUM, BoundNetLog()); + ASSERT_TRUE(spdy_stream1.get() != NULL); EXPECT_EQ(0u, spdy_stream1->stream_id()); scoped_ptr headers(new SpdyHeaderBlock); @@ -1887,11 +1840,10 @@ TEST_F(SpdySessionSpdy2Test, TestYieldingDuringAsyncReadData) { scoped_refptr session = CreateInitializedSession(); - scoped_refptr spdy_stream1; - TestCompletionCallback callback1; GURL url1("http://www.google.com"); - EXPECT_EQ(OK, session->CreateStream(url1, MEDIUM, &spdy_stream1, - BoundNetLog(), callback1.callback())); + scoped_refptr spdy_stream1 = + CreateStreamSynchronously(session, url1, MEDIUM, BoundNetLog()); + ASSERT_TRUE(spdy_stream1.get() != NULL); EXPECT_EQ(0u, spdy_stream1->stream_id()); scoped_ptr headers(new SpdyHeaderBlock); @@ -1963,11 +1915,10 @@ TEST_F(SpdySessionSpdy2Test, GoAwayWhileInDoLoop) { scoped_refptr session = CreateInitializedSession(); - scoped_refptr spdy_stream1; - TestCompletionCallback callback1; GURL url1("http://www.google.com"); - EXPECT_EQ(OK, session->CreateStream(url1, MEDIUM, &spdy_stream1, - BoundNetLog(), callback1.callback())); + scoped_refptr spdy_stream1 = + CreateStreamSynchronously(session, url1, MEDIUM, BoundNetLog()); + ASSERT_TRUE(spdy_stream1.get() != NULL); EXPECT_EQ(0u, spdy_stream1->stream_id()); scoped_ptr headers(new SpdyHeaderBlock); diff --git a/net/spdy/spdy_session_spdy3_unittest.cc b/net/spdy/spdy_session_spdy3_unittest.cc index 6dc72d4..a0d8583 100644 --- a/net/spdy/spdy_session_spdy3_unittest.cc +++ b/net/spdy/spdy_session_spdy3_unittest.cc @@ -22,6 +22,7 @@ #include "net/spdy/spdy_session_test_util.h" #include "net/spdy/spdy_stream.h" #include "net/spdy/spdy_stream_test_util.h" +#include "net/spdy/spdy_test_util_common.h" #include "net/spdy/spdy_test_util_spdy3.h" #include "testing/platform_test.h" @@ -277,10 +278,10 @@ TEST_F(SpdySessionSpdy3Test, ClientPing) { scoped_refptr session = CreateInitializedSession(); - scoped_refptr spdy_stream1; + scoped_refptr spdy_stream1 = + CreateStreamSynchronously(session, test_url_, MEDIUM, BoundNetLog()); + ASSERT_TRUE(spdy_stream1.get() != NULL); TestCompletionCallback callback1; - EXPECT_EQ(OK, session->CreateStream(test_url_, MEDIUM, &spdy_stream1, - BoundNetLog(), callback1.callback())); scoped_ptr delegate( new TestSpdyStreamDelegate(callback1.callback())); @@ -333,10 +334,10 @@ TEST_F(SpdySessionSpdy3Test, ServerPing) { scoped_refptr session = CreateInitializedSession(); - scoped_refptr spdy_stream1; + scoped_refptr spdy_stream1 = + CreateStreamSynchronously(session, test_url_, MEDIUM, BoundNetLog()); + ASSERT_TRUE(spdy_stream1.get() != NULL); TestCompletionCallback callback1; - EXPECT_EQ(OK, session->CreateStream(test_url_, MEDIUM, &spdy_stream1, - BoundNetLog(), callback1.callback())); scoped_ptr delegate( new TestSpdyStreamDelegate(callback1.callback())); spdy_stream1->SetDelegate(delegate.get()); @@ -429,10 +430,10 @@ TEST_F(SpdySessionSpdy3Test, FailedPing) { scoped_refptr session = CreateInitializedSession(); - scoped_refptr spdy_stream1; + scoped_refptr spdy_stream1 = + CreateStreamSynchronously(session, test_url_, MEDIUM, BoundNetLog()); + ASSERT_TRUE(spdy_stream1.get() != NULL); TestCompletionCallback callback1; - EXPECT_EQ(OK, session->CreateStream(test_url_, MEDIUM, &spdy_stream1, - BoundNetLog(), callback1.callback())); scoped_ptr delegate( new TestSpdyStreamDelegate(callback1.callback())); spdy_stream1->SetDelegate(delegate.get()); @@ -466,40 +467,6 @@ TEST_F(SpdySessionSpdy3Test, FailedPing) { session = NULL; } -class StreamReleaserCallback : public TestCompletionCallbackBase { - public: - StreamReleaserCallback(SpdySession* session, - SpdyStream* first_stream) - : session_(session), - first_stream_(first_stream), - ALLOW_THIS_IN_INITIALIZER_LIST(callback_( - base::Bind(&StreamReleaserCallback::OnComplete, - base::Unretained(this)))) { - } - - virtual ~StreamReleaserCallback() {} - - scoped_refptr* stream() { return &stream_; } - - const CompletionCallback& callback() const { return callback_; } - - private: - void OnComplete(int result) { - session_->CloseSessionOnError(ERR_FAILED, false, "On complete."); - session_ = NULL; - first_stream_->Cancel(); - first_stream_ = NULL; - stream_->Cancel(); - stream_ = NULL; - SetResult(result); - } - - scoped_refptr session_; - scoped_refptr first_stream_; - scoped_refptr stream_; - CompletionCallback callback_; -}; - // TODO(kristianm): Could also test with more sessions where some are idle, // and more than one session to a HostPortPair. TEST_F(SpdySessionSpdy3Test, CloseIdleSessions) { @@ -510,33 +477,30 @@ TEST_F(SpdySessionSpdy3Test, CloseIdleSessions) { HostPortPair test_host_port_pair1(kTestHost1, 80); HostPortProxyPair pair1(test_host_port_pair1, ProxyServer::Direct()); scoped_refptr session1 = GetSession(pair1); - scoped_refptr spdy_stream1; - TestCompletionCallback callback1; GURL url1(kTestHost1); - EXPECT_EQ(OK, session1->CreateStream(url1, MEDIUM, &spdy_stream1, - BoundNetLog(), callback1.callback())); + scoped_refptr spdy_stream1 = + CreateStreamSynchronously(session1, url1, MEDIUM, BoundNetLog()); + ASSERT_TRUE(spdy_stream1.get() != NULL); // Set up session 2 const std::string kTestHost2("http://www.b.com"); HostPortPair test_host_port_pair2(kTestHost2, 80); HostPortProxyPair pair2(test_host_port_pair2, ProxyServer::Direct()); scoped_refptr session2 = GetSession(pair2); - scoped_refptr spdy_stream2; - TestCompletionCallback callback2; GURL url2(kTestHost2); - EXPECT_EQ(OK, session2->CreateStream(url2, MEDIUM, &spdy_stream2, - BoundNetLog(), callback2.callback())); + scoped_refptr spdy_stream2 = + CreateStreamSynchronously(session2, url2, MEDIUM, BoundNetLog()); + ASSERT_TRUE(spdy_stream2.get() != NULL); // Set up session 3 const std::string kTestHost3("http://www.c.com"); HostPortPair test_host_port_pair3(kTestHost3, 80); HostPortProxyPair pair3(test_host_port_pair3, ProxyServer::Direct()); scoped_refptr session3 = GetSession(pair3); - scoped_refptr spdy_stream3; - TestCompletionCallback callback3; GURL url3(kTestHost3); - EXPECT_EQ(OK, session3->CreateStream(url3, MEDIUM, &spdy_stream3, - BoundNetLog(), callback3.callback())); + scoped_refptr spdy_stream3 = + CreateStreamSynchronously(session3, url3, MEDIUM, BoundNetLog()); + ASSERT_TRUE(spdy_stream3.get() != NULL); // All sessions are active and not closed EXPECT_TRUE(session1->is_active()); @@ -633,17 +597,17 @@ TEST_F(SpdySessionSpdy3Test, OnSettings) { scoped_refptr session = CreateInitializedSession(); // Create 2 streams. First will succeed. Second will be pending. - scoped_refptr spdy_stream1; - TestCompletionCallback callback1; - EXPECT_EQ(OK, - session->CreateStream(test_url_, MEDIUM, &spdy_stream1, - BoundNetLog(), callback1.callback())); + scoped_refptr spdy_stream1 = + CreateStreamSynchronously(session, test_url_, MEDIUM, BoundNetLog()); + ASSERT_TRUE(spdy_stream1.get() != NULL); StreamReleaserCallback stream_releaser(session, spdy_stream1); + SpdyStreamRequest request; ASSERT_EQ(ERR_IO_PENDING, - session->CreateStream(test_url_, MEDIUM, stream_releaser.stream(), - BoundNetLog(), stream_releaser.callback())); + request.StartRequest(session, test_url_, MEDIUM, + BoundNetLog(), + stream_releaser.MakeCallback(&request))); // Make sure |stream_releaser| holds the last refs. session = NULL; @@ -683,25 +647,26 @@ TEST_F(SpdySessionSpdy3Test, CancelPendingCreateStream) { scoped_refptr session = CreateInitializedSession(); + // Create 2 streams. First will succeed. Second will be pending. + scoped_refptr spdy_stream1 = + CreateStreamSynchronously(session, test_url_, MEDIUM, BoundNetLog()); + ASSERT_TRUE(spdy_stream1.get() != NULL); + // Use scoped_ptr to let us invalidate the memory when we want to, to trigger // a valgrind error if the callback is invoked when it's not supposed to be. scoped_ptr callback(new TestCompletionCallback); - // Create 2 streams. First will succeed. Second will be pending. - scoped_refptr spdy_stream1; - ASSERT_EQ(OK, session->CreateStream(test_url_, MEDIUM, &spdy_stream1, - BoundNetLog(), callback->callback())); - - scoped_refptr spdy_stream2; + SpdyStreamRequest request; ASSERT_EQ(ERR_IO_PENDING, - session->CreateStream(test_url_, MEDIUM, &spdy_stream2, - BoundNetLog(), callback->callback())); + request.StartRequest(session, test_url_, MEDIUM, + BoundNetLog(), + callback->callback())); // Release the first one, this will allow the second to be created. spdy_stream1->Cancel(); spdy_stream1 = NULL; - session->CancelPendingCreateStreams(&spdy_stream2); + request.CancelRequest(); callback.reset(); // Should not crash when running the pending callback. @@ -1044,17 +1009,16 @@ TEST_F(SpdySessionSpdy3Test, OutOfOrderSynStreams) { scoped_refptr session = CreateInitializedSession(); - scoped_refptr spdy_stream1; - TestCompletionCallback callback1; GURL url("http://www.google.com"); - EXPECT_EQ(OK, session->CreateStream(url, LOWEST, &spdy_stream1, - BoundNetLog(), callback1.callback())); + + scoped_refptr spdy_stream1 = + CreateStreamSynchronously(session, url, LOWEST, BoundNetLog()); + ASSERT_TRUE(spdy_stream1.get() != NULL); EXPECT_EQ(0u, spdy_stream1->stream_id()); - scoped_refptr spdy_stream2; - TestCompletionCallback callback2; - EXPECT_EQ(OK, session->CreateStream(url, HIGHEST, &spdy_stream2, - BoundNetLog(), callback2.callback())); + scoped_refptr spdy_stream2 = + CreateStreamSynchronously(session, url, HIGHEST, BoundNetLog()); + ASSERT_TRUE(spdy_stream2.get() != NULL); EXPECT_EQ(0u, spdy_stream2->stream_id()); scoped_ptr headers(new SpdyHeaderBlock); @@ -1117,18 +1081,16 @@ TEST_F(SpdySessionSpdy3Test, CancelStream) { scoped_refptr session = CreateInitializedSession(); - scoped_refptr spdy_stream1; - TestCompletionCallback callback1; GURL url1("http://www.google.com"); - EXPECT_EQ(OK, session->CreateStream(url1, HIGHEST, &spdy_stream1, - BoundNetLog(), callback1.callback())); + scoped_refptr spdy_stream1 = + CreateStreamSynchronously(session, url1, HIGHEST, BoundNetLog()); + ASSERT_TRUE(spdy_stream1.get() != NULL); EXPECT_EQ(0u, spdy_stream1->stream_id()); - scoped_refptr spdy_stream2; - TestCompletionCallback callback2; GURL url2("http://www.google.com"); - EXPECT_EQ(OK, session->CreateStream(url2, LOWEST, &spdy_stream2, - BoundNetLog(), callback2.callback())); + scoped_refptr spdy_stream2 = + CreateStreamSynchronously(session, url2, LOWEST, BoundNetLog()); + ASSERT_TRUE(spdy_stream2.get() != NULL); EXPECT_EQ(0u, spdy_stream2->stream_id()); scoped_ptr headers(new SpdyHeaderBlock); @@ -1192,18 +1154,16 @@ TEST_F(SpdySessionSpdy3Test, CloseSessionWithTwoCreatedStreams) { scoped_refptr session = CreateInitializedSession(); - scoped_refptr spdy_stream1; - TestCompletionCallback callback1; GURL url1("http://www.google.com"); - EXPECT_EQ(OK, session->CreateStream(url1, HIGHEST, &spdy_stream1, - BoundNetLog(), callback1.callback())); + scoped_refptr spdy_stream1 = + CreateStreamSynchronously(session, url1, HIGHEST, BoundNetLog()); + ASSERT_TRUE(spdy_stream1.get() != NULL); EXPECT_EQ(0u, spdy_stream1->stream_id()); - scoped_refptr spdy_stream2; - TestCompletionCallback callback2; GURL url2("http://www.google.com"); - EXPECT_EQ(OK, session->CreateStream(url2, LOWEST, &spdy_stream2, - BoundNetLog(), callback2.callback())); + scoped_refptr spdy_stream2 = + CreateStreamSynchronously(session, url2, LOWEST, BoundNetLog()); + ASSERT_TRUE(spdy_stream2.get() != NULL); EXPECT_EQ(0u, spdy_stream2->stream_id()); scoped_ptr headers(new SpdyHeaderBlock); @@ -1438,27 +1398,27 @@ TEST_F(SpdySessionSpdy3Test, CloseTwoStalledCreateStream) { // Read the settings frame. data.RunFor(1); - scoped_refptr spdy_stream1; - TestCompletionCallback callback1; GURL url1("http://www.google.com"); - EXPECT_EQ(OK, session->CreateStream(url1, LOWEST, &spdy_stream1, - BoundNetLog(), callback1.callback())); - + scoped_refptr spdy_stream1 = + CreateStreamSynchronously(session, url1, LOWEST, BoundNetLog()); + ASSERT_TRUE(spdy_stream1.get() != NULL); EXPECT_EQ(0u, spdy_stream1->stream_id()); - scoped_refptr spdy_stream2; TestCompletionCallback callback2; GURL url2("http://www.google.com"); - EXPECT_EQ(ERR_IO_PENDING, - session->CreateStream(url2, LOWEST, &spdy_stream2, - BoundNetLog(), callback2.callback())); + SpdyStreamRequest request2; + ASSERT_EQ(ERR_IO_PENDING, + request2.StartRequest(session, url2, LOWEST, + BoundNetLog(), + callback2.callback())); - scoped_refptr spdy_stream3; TestCompletionCallback callback3; GURL url3("http://www.google.com"); - EXPECT_EQ(ERR_IO_PENDING, - session->CreateStream(url3, LOWEST, &spdy_stream3, - BoundNetLog(), callback3.callback())); + SpdyStreamRequest request3; + ASSERT_EQ(ERR_IO_PENDING, + request3.StartRequest(session, url3, LOWEST, + BoundNetLog(), + callback3.callback())); EXPECT_EQ(1u, session->num_active_streams() + session->num_created_streams()); EXPECT_EQ(2u, session->pending_create_stream_queues(LOWEST)); @@ -1478,33 +1438,33 @@ TEST_F(SpdySessionSpdy3Test, CloseTwoStalledCreateStream) { EXPECT_TRUE(spdy_stream1->HasUrl()); spdy_stream1->SendRequest(false); - // Run until 1st stream is closed. + // Run until 1st stream is closed and 2nd one is opened. EXPECT_EQ(0u, spdy_stream1->stream_id()); data.RunFor(3); EXPECT_EQ(1u, spdy_stream1->stream_id()); - EXPECT_EQ(1u, session->num_active_streams() + session->num_created_streams()); - EXPECT_EQ(1u, session->pending_create_stream_queues(LOWEST)); + EXPECT_EQ(2u, session->num_active_streams() + session->num_created_streams()); + EXPECT_EQ(0u, session->pending_create_stream_queues(LOWEST)); - EXPECT_TRUE(spdy_stream2.get() != NULL); - spdy_stream2->set_spdy_headers(headers2.Pass()); - EXPECT_TRUE(spdy_stream2->HasUrl()); - spdy_stream2->SendRequest(false); + scoped_refptr stream2 = request2.ReleaseStream(); + stream2->set_spdy_headers(headers2.Pass()); + EXPECT_TRUE(stream2->HasUrl()); + stream2->SendRequest(false); // Run until 2nd stream is closed. - EXPECT_EQ(0u, spdy_stream2->stream_id()); + EXPECT_EQ(0u, stream2->stream_id()); data.RunFor(3); - EXPECT_EQ(3u, spdy_stream2->stream_id()); + EXPECT_EQ(3u, stream2->stream_id()); EXPECT_EQ(1u, session->num_active_streams() + session->num_created_streams()); EXPECT_EQ(0u, session->pending_create_stream_queues(LOWEST)); - EXPECT_TRUE(spdy_stream3.get() != NULL); - spdy_stream3->set_spdy_headers(headers3.Pass()); - EXPECT_TRUE(spdy_stream3->HasUrl()); - spdy_stream3->SendRequest(false); + scoped_refptr stream3 = request3.ReleaseStream(); + stream3->set_spdy_headers(headers3.Pass()); + EXPECT_TRUE(stream3->HasUrl()); + stream3->SendRequest(false); - EXPECT_EQ(0u, spdy_stream3->stream_id()); + EXPECT_EQ(0u, stream3->stream_id()); data.RunFor(4); - EXPECT_EQ(5u, spdy_stream3->stream_id()); + EXPECT_EQ(5u, stream3->stream_id()); EXPECT_EQ(0u, session->num_active_streams() + session->num_created_streams()); EXPECT_EQ(0u, session->pending_create_stream_queues(LOWEST)); } @@ -1536,27 +1496,27 @@ TEST_F(SpdySessionSpdy3Test, CancelTwoStalledCreateStream) { scoped_refptr session = CreateInitializedSession(); - scoped_refptr spdy_stream1; - TestCompletionCallback callback1; GURL url1("http://www.google.com"); - ASSERT_EQ(OK, - session->CreateStream(url1, LOWEST, &spdy_stream1, - BoundNetLog(), callback1.callback())); + scoped_refptr spdy_stream1 = + CreateStreamSynchronously(session, url1, LOWEST, BoundNetLog()); + ASSERT_TRUE(spdy_stream1.get() != NULL); EXPECT_EQ(0u, spdy_stream1->stream_id()); - scoped_refptr spdy_stream2; TestCompletionCallback callback2; GURL url2("http://www.google.com"); + SpdyStreamRequest request2; ASSERT_EQ(ERR_IO_PENDING, - session->CreateStream(url2, LOWEST, &spdy_stream2, - BoundNetLog(), callback2.callback())); + request2.StartRequest(session, url2, LOWEST, + BoundNetLog(), + callback2.callback())); - scoped_refptr spdy_stream3; TestCompletionCallback callback3; GURL url3("http://www.google.com"); + SpdyStreamRequest request3; ASSERT_EQ(ERR_IO_PENDING, - session->CreateStream(url3, LOWEST, &spdy_stream3, - BoundNetLog(), callback3.callback())); + request3.StartRequest(session, url3, LOWEST, + BoundNetLog(), + callback3.callback())); EXPECT_EQ(1u, session->num_active_streams() + session->num_created_streams()); EXPECT_EQ(2u, session->pending_create_stream_queues(LOWEST)); @@ -1565,23 +1525,18 @@ TEST_F(SpdySessionSpdy3Test, CancelTwoStalledCreateStream) { EXPECT_TRUE(spdy_stream1.get() != NULL); spdy_stream1->Cancel(); spdy_stream1 = NULL; - session->CancelPendingCreateStreams(&spdy_stream1); - EXPECT_EQ(1u, session->num_active_streams() + session->num_created_streams()); - EXPECT_EQ(1u, session->pending_create_stream_queues(LOWEST)); + + callback2.WaitForResult(); + EXPECT_EQ(2u, session->num_active_streams() + session->num_created_streams()); + EXPECT_EQ(0u, session->pending_create_stream_queues(LOWEST)); // Cancel the second stream, this will allow the third stream to be created. - EXPECT_TRUE(spdy_stream2.get() != NULL); - spdy_stream2->Cancel(); - spdy_stream2 = NULL; - session->CancelPendingCreateStreams(&spdy_stream2); + request2.ReleaseStream()->Cancel(); EXPECT_EQ(1u, session->num_active_streams() + session->num_created_streams()); EXPECT_EQ(0u, session->pending_create_stream_queues(LOWEST)); // Cancel the third stream. - EXPECT_TRUE(spdy_stream3.get() != NULL); - spdy_stream3->Cancel(); - spdy_stream3 = NULL; - session->CancelPendingCreateStreams(&spdy_stream3); + request3.ReleaseStream()->Cancel(); EXPECT_EQ(0u, session->num_active_streams() + session->num_created_streams()); EXPECT_EQ(0u, session->pending_create_stream_queues(LOWEST)); } @@ -1738,10 +1693,10 @@ TEST_F(SpdySessionSpdy3Test, UpdateStreamsSendWindowSize) { CreateDeterministicNetworkSession(); scoped_refptr session = CreateInitializedSession(); - scoped_refptr spdy_stream1; + scoped_refptr spdy_stream1 = + CreateStreamSynchronously(session, test_url_, MEDIUM, BoundNetLog()); + ASSERT_TRUE(spdy_stream1.get() != NULL); TestCompletionCallback callback1; - EXPECT_EQ(OK, session->CreateStream(test_url_, MEDIUM, &spdy_stream1, - BoundNetLog(), callback1.callback())); EXPECT_NE(spdy_stream1->send_window_size(), window_size); data->RunFor(1); // Process the SETTINGS frame, but not the EOF @@ -1753,10 +1708,9 @@ TEST_F(SpdySessionSpdy3Test, UpdateStreamsSendWindowSize) { spdy_stream1->Cancel(); spdy_stream1 = NULL; - scoped_refptr spdy_stream2; - EXPECT_EQ(OK, session->CreateStream(test_url_, MEDIUM, &spdy_stream2, - BoundNetLog(), callback1.callback())); - + scoped_refptr spdy_stream2 = + CreateStreamSynchronously(session, test_url_, MEDIUM, BoundNetLog()); + ASSERT_TRUE(spdy_stream2.get() != NULL); EXPECT_EQ(spdy_stream2->send_window_size(), window_size); spdy_stream2->Cancel(); spdy_stream2 = NULL; @@ -1815,11 +1769,10 @@ TEST_F(SpdySessionSpdy3Test, ReadDataWithoutYielding) { scoped_refptr session = CreateInitializedSession(); - scoped_refptr spdy_stream1; - TestCompletionCallback callback1; GURL url1("http://www.google.com"); - EXPECT_EQ(OK, session->CreateStream(url1, MEDIUM, &spdy_stream1, - BoundNetLog(), callback1.callback())); + scoped_refptr spdy_stream1 = + CreateStreamSynchronously(session, url1, MEDIUM, BoundNetLog()); + ASSERT_TRUE(spdy_stream1.get() != NULL); EXPECT_EQ(0u, spdy_stream1->stream_id()); scoped_ptr headers(new SpdyHeaderBlock); @@ -1906,11 +1859,10 @@ TEST_F(SpdySessionSpdy3Test, TestYieldingDuringReadData) { scoped_refptr session = CreateInitializedSession(); - scoped_refptr spdy_stream1; - TestCompletionCallback callback1; GURL url1("http://www.google.com"); - EXPECT_EQ(OK, session->CreateStream(url1, MEDIUM, &spdy_stream1, - BoundNetLog(), callback1.callback())); + scoped_refptr spdy_stream1 = + CreateStreamSynchronously(session, url1, MEDIUM, BoundNetLog()); + ASSERT_TRUE(spdy_stream1.get() != NULL); EXPECT_EQ(0u, spdy_stream1->stream_id()); scoped_ptr headers(new SpdyHeaderBlock); @@ -2020,11 +1972,10 @@ TEST_F(SpdySessionSpdy3Test, TestYieldingDuringAsyncReadData) { scoped_refptr session = CreateInitializedSession(); - scoped_refptr spdy_stream1; - TestCompletionCallback callback1; GURL url1("http://www.google.com"); - EXPECT_EQ(OK, session->CreateStream(url1, MEDIUM, &spdy_stream1, - BoundNetLog(), callback1.callback())); + scoped_refptr spdy_stream1 = + CreateStreamSynchronously(session, url1, MEDIUM, BoundNetLog()); + ASSERT_TRUE(spdy_stream1.get() != NULL); EXPECT_EQ(0u, spdy_stream1->stream_id()); scoped_ptr headers(new SpdyHeaderBlock); @@ -2096,11 +2047,10 @@ TEST_F(SpdySessionSpdy3Test, GoAwayWhileInDoLoop) { scoped_refptr session = CreateInitializedSession(); - scoped_refptr spdy_stream1; - TestCompletionCallback callback1; GURL url1("http://www.google.com"); - EXPECT_EQ(OK, session->CreateStream(url1, MEDIUM, &spdy_stream1, - BoundNetLog(), callback1.callback())); + scoped_refptr spdy_stream1 = + CreateStreamSynchronously(session, url1, MEDIUM, BoundNetLog()); + ASSERT_TRUE(spdy_stream1.get() != NULL); EXPECT_EQ(0u, spdy_stream1->stream_id()); scoped_ptr headers(new SpdyHeaderBlock); @@ -2405,11 +2355,10 @@ TEST_F(SpdySessionSpdy3Test, SessionFlowControlEndToEnd31) { scoped_refptr session = CreateInitializedSession(); - scoped_refptr stream; - TestCompletionCallback callback1; GURL url1("http://www.google.com"); - EXPECT_EQ(OK, session->CreateStream(url1, MEDIUM, &stream, - BoundNetLog(), callback1.callback())); + scoped_refptr stream = + CreateStreamSynchronously(session, url1, MEDIUM, BoundNetLog()); + ASSERT_TRUE(stream.get() != NULL); EXPECT_EQ(0u, stream->stream_id()); scoped_refptr buf(new IOBufferWithSize(msg_data_size)); diff --git a/net/spdy/spdy_stream_spdy2_unittest.cc b/net/spdy/spdy_stream_spdy2_unittest.cc index 4473d02..1e85308 100644 --- a/net/spdy/spdy_stream_spdy2_unittest.cc +++ b/net/spdy/spdy_stream_spdy2_unittest.cc @@ -10,6 +10,7 @@ #include "net/spdy/spdy_http_utils.h" #include "net/spdy/spdy_session.h" #include "net/spdy/spdy_stream_test_util.h" +#include "net/spdy/spdy_test_util_common.h" #include "net/spdy/spdy_test_util_spdy2.h" #include "net/spdy/spdy_websocket_test_util_spdy2.h" #include "testing/gtest/include/gtest/gtest.h" @@ -129,11 +130,9 @@ TEST_F(SpdyStreamSpdy2Test, SendDataAfterOpen) { BoundNetLog())); session->InitializeWithSocket(connection.release(), false, OK); - scoped_refptr stream; - ASSERT_EQ( - OK, - session->CreateStream(url, LOWEST, &stream, BoundNetLog(), - CompletionCallback())); + scoped_refptr stream = + CreateStreamSynchronously(session, url, LOWEST, BoundNetLog()); + ASSERT_TRUE(stream.get() != NULL); scoped_refptr buf(new IOBufferWithSize(8)); memcpy(buf->data(), "\0hello!\xff", 8); TestCompletionCallback callback; @@ -223,11 +222,9 @@ TEST_F(SpdyStreamSpdy2Test, SendHeaderAndDataAfterOpen) { BoundNetLog())); session->InitializeWithSocket(connection.release(), false, OK); - scoped_refptr stream; - ASSERT_EQ( - OK, - session->CreateStream(url, HIGHEST, &stream, BoundNetLog(), - CompletionCallback())); + scoped_refptr stream = + CreateStreamSynchronously(session, url, HIGHEST, BoundNetLog()); + ASSERT_TRUE(stream.get() != NULL); scoped_refptr buf(new IOBufferWithSize(6)); memcpy(buf->data(), "hello!", 6); TestCompletionCallback callback; @@ -402,11 +399,9 @@ TEST_F(SpdyStreamSpdy2Test, StreamError) { log.bound())); session->InitializeWithSocket(connection.release(), false, OK); - scoped_refptr stream; - ASSERT_EQ( - OK, - session->CreateStream(url, LOWEST, &stream, log.bound(), - CompletionCallback())); + scoped_refptr stream = + CreateStreamSynchronously(session, url, LOWEST, log.bound()); + ASSERT_TRUE(stream.get() != NULL); scoped_refptr buf(new IOBufferWithSize(8)); memcpy(buf->data(), "\0hello!\xff", 8); TestCompletionCallback callback; diff --git a/net/spdy/spdy_stream_spdy3_unittest.cc b/net/spdy/spdy_stream_spdy3_unittest.cc index 0bb7da6..e0cbc3a 100644 --- a/net/spdy/spdy_stream_spdy3_unittest.cc +++ b/net/spdy/spdy_stream_spdy3_unittest.cc @@ -10,6 +10,7 @@ #include "net/spdy/spdy_http_utils.h" #include "net/spdy/spdy_session.h" #include "net/spdy/spdy_stream_test_util.h" +#include "net/spdy/spdy_test_util_common.h" #include "net/spdy/spdy_test_util_spdy3.h" #include "net/spdy/spdy_websocket_test_util_spdy3.h" #include "testing/gtest/include/gtest/gtest.h" @@ -130,11 +131,10 @@ TEST_F(SpdyStreamSpdy3Test, SendDataAfterOpen) { BoundNetLog())); session->InitializeWithSocket(connection.release(), false, OK); - scoped_refptr stream; - ASSERT_EQ( - OK, - session->CreateStream(url, LOWEST, &stream, BoundNetLog(), - CompletionCallback())); + + scoped_refptr stream = + CreateStreamSynchronously(session, url, LOWEST, BoundNetLog()); + ASSERT_TRUE(stream.get() != NULL); scoped_refptr buf(new IOBufferWithSize(8)); memcpy(buf->data(), "\0hello!\xff", 8); TestCompletionCallback callback; @@ -224,11 +224,9 @@ TEST_F(SpdyStreamSpdy3Test, SendHeaderAndDataAfterOpen) { BoundNetLog())); session->InitializeWithSocket(connection.release(), false, OK); - scoped_refptr stream; - ASSERT_EQ( - OK, - session->CreateStream(url, HIGHEST, &stream, BoundNetLog(), - CompletionCallback())); + scoped_refptr stream = + CreateStreamSynchronously(session, url, HIGHEST, BoundNetLog()); + ASSERT_TRUE(stream.get() != NULL); scoped_refptr buf(new IOBufferWithSize(6)); memcpy(buf->data(), "hello!", 6); TestCompletionCallback callback; @@ -407,11 +405,9 @@ TEST_F(SpdyStreamSpdy3Test, StreamError) { log.bound())); session->InitializeWithSocket(connection.release(), false, OK); - scoped_refptr stream; - ASSERT_EQ( - OK, - session->CreateStream(url, LOWEST, &stream, log.bound(), - CompletionCallback())); + scoped_refptr stream = + CreateStreamSynchronously(session, url, LOWEST, log.bound()); + ASSERT_TRUE(stream.get() != NULL); scoped_refptr buf(new IOBufferWithSize(8)); memcpy(buf->data(), "\0hello!\xff", 8); TestCompletionCallback callback; @@ -509,11 +505,9 @@ TEST_F(SpdyStreamSpdy3Test, IncreaseSendWindowSizeOverflow) { log.bound())); session->InitializeWithSocket(connection.release(), false, OK); - scoped_refptr stream; - ASSERT_EQ( - OK, - session->CreateStream(url, LOWEST, &stream, log.bound(), - CompletionCallback())); + scoped_refptr stream = + CreateStreamSynchronously(session, url, LOWEST, log.bound()); + ASSERT_TRUE(stream.get() != NULL); TestCompletionCallback callback; scoped_ptr delegate( diff --git a/net/spdy/spdy_test_util_common.cc b/net/spdy/spdy_test_util_common.cc index e46ef9f..0f38c1d 100644 --- a/net/spdy/spdy_test_util_common.cc +++ b/net/spdy/spdy_test_util_common.cc @@ -8,6 +8,8 @@ #include "base/compiler_specific.h" #include "net/spdy/buffered_spdy_framer.h" +#include "net/spdy/spdy_session.h" +#include "net/spdy/spdy_stream.h" namespace net { @@ -77,4 +79,40 @@ bool GetSpdyPriority(int version, return true; } -} // namespace net +scoped_refptr CreateStreamSynchronously( + const scoped_refptr& session, + const GURL& url, + RequestPriority priority, + const BoundNetLog& net_log) { + SpdyStreamRequest stream_request; + int rv = stream_request.StartRequest(session, url, priority, net_log, + CompletionCallback()); + return (rv == OK) ? stream_request.ReleaseStream() : NULL; +} + +StreamReleaserCallback::StreamReleaserCallback( + SpdySession* session, + SpdyStream* first_stream) + : session_(session), + first_stream_(first_stream) {} + +StreamReleaserCallback::~StreamReleaserCallback() {} + +CompletionCallback StreamReleaserCallback::MakeCallback( + SpdyStreamRequest* request) { + return base::Bind(&StreamReleaserCallback::OnComplete, + base::Unretained(this), + request); +} + +void StreamReleaserCallback::OnComplete( + SpdyStreamRequest* request, int result) { + session_->CloseSessionOnError(ERR_FAILED, false, "On complete."); + session_ = NULL; + first_stream_->Cancel(); + first_stream_ = NULL; + request->ReleaseStream()->Cancel(); + SetResult(result); +} + +} // namespace net diff --git a/net/spdy/spdy_test_util_common.h b/net/spdy/spdy_test_util_common.h index 4f45bc6..ff857ca 100644 --- a/net/spdy/spdy_test_util_common.h +++ b/net/spdy/spdy_test_util_common.h @@ -5,16 +5,55 @@ #ifndef NET_SPDY_SPDY_TEST_UTIL_COMMON_H_ #define NET_SPDY_SPDY_TEST_UTIL_COMMON_H_ +#include "base/memory/ref_counted.h" +#include "net/base/completion_callback.h" +#include "net/base/request_priority.h" +#include "net/base/test_completion_callback.h" #include "net/spdy/spdy_protocol.h" +class GURL; + namespace net { +class BoundNetLog; +class SpdySession; +class SpdyStream; +class SpdyStreamRequest; + // Returns the SpdyPriority embedded in the given frame. Returns true // and fills in |priority| on success. bool GetSpdyPriority(int version, const SpdyFrame& frame, SpdyPriority* priority); +// Tries to create a stream in |session| synchronously. Returns NULL +// on failure. +scoped_refptr CreateStreamSynchronously( + const scoped_refptr& session, + const GURL& url, + RequestPriority priority, + const BoundNetLog& net_log); + +// Helper class used by some tests to release two streams as soon as +// one is created. +class StreamReleaserCallback : public TestCompletionCallbackBase { + public: + StreamReleaserCallback(SpdySession* session, + SpdyStream* first_stream); + + virtual ~StreamReleaserCallback(); + + // Returns a callback that releases |request|'s stream as well as + // |first_stream|. + CompletionCallback MakeCallback(SpdyStreamRequest* request); + + private: + void OnComplete(SpdyStreamRequest* request, int result); + + scoped_refptr session_; + scoped_refptr first_stream_; +}; + } // namespace net #endif // NET_SPDY_SPDY_TEST_UTIL_COMMON_H_ diff --git a/net/spdy/spdy_websocket_stream.cc b/net/spdy/spdy_websocket_stream.cc index 6323b71..fe02b54 100644 --- a/net/spdy/spdy_websocket_stream.cc +++ b/net/spdy/spdy_websocket_stream.cc @@ -42,16 +42,17 @@ int SpdyWebSocketStream::InitializeStream(const GURL& url, if (spdy_session_->IsClosed()) return ERR_SOCKET_NOT_CONNECTED; - int result = spdy_session_->CreateStream( - url, request_priority, &stream_, net_log, + int rv = stream_request_.StartRequest( + spdy_session_, url, request_priority, net_log, base::Bind(&SpdyWebSocketStream::OnSpdyStreamCreated, base::Unretained(this))); - if (result == OK) { + if (rv == OK) { + stream_ = stream_request_.ReleaseStream(); DCHECK(stream_); stream_->SetDelegate(this); } - return result; + return rv; } int SpdyWebSocketStream::SendRequest(scoped_ptr headers) { @@ -77,8 +78,6 @@ int SpdyWebSocketStream::SendData(const char* data, int length) { } void SpdyWebSocketStream::Close() { - if (spdy_session_) - spdy_session_->CancelPendingCreateStreams(&stream_); if (stream_) stream_->Close(); } @@ -137,6 +136,7 @@ void SpdyWebSocketStream::OnClose(int status) { void SpdyWebSocketStream::OnSpdyStreamCreated(int result) { DCHECK_NE(ERR_IO_PENDING, result); if (result == OK) { + stream_ = stream_request_.ReleaseStream(); DCHECK(stream_); stream_->SetDelegate(this); } diff --git a/net/spdy/spdy_websocket_stream.h b/net/spdy/spdy_websocket_stream.h index 5ab4c48..c003a4b 100644 --- a/net/spdy/spdy_websocket_stream.h +++ b/net/spdy/spdy_websocket_stream.h @@ -14,6 +14,7 @@ #include "net/base/request_priority.h" #include "net/spdy/spdy_framer.h" #include "net/spdy/spdy_header_block.h" +#include "net/spdy/spdy_session.h" #include "net/spdy/spdy_stream.h" namespace net { @@ -91,6 +92,7 @@ class NET_EXPORT_PRIVATE SpdyWebSocketStream void OnSpdyStreamCreated(int status); + SpdyStreamRequest stream_request_; scoped_refptr stream_; scoped_refptr spdy_session_; Delegate* delegate_; -- cgit v1.1