From 33d24423df4b30680edc9a473b945a8ca7b50127 Mon Sep 17 00:00:00 2001 From: mmenke Date: Tue, 19 May 2015 12:41:09 -0700 Subject: [PATCH] Clean up weird ClientSocketPoolBase unit tests. The tests were re-entrantly reusing a TestCompletionCallback subclass in a weird manner, which has now been fixed. Also fix a WebSocket test modelled after them. BUG=114130 Review URL: https://codereview.chromium.org/1132313004 Cr-Commit-Position: refs/heads/master@{#330585} --- net/socket/client_socket_pool_base_unittest.cc | 155 +++++++-------------- ...socket_transport_client_socket_pool_unittest.cc | 92 +++++------- 2 files changed, 88 insertions(+), 159 deletions(-) diff --git a/net/socket/client_socket_pool_base_unittest.cc b/net/socket/client_socket_pool_base_unittest.cc index cfa9c6e115f7..ac75e448aa39 100644 --- a/net/socket/client_socket_pool_base_unittest.cc +++ b/net/socket/client_socket_pool_base_unittest.cc @@ -9,6 +9,7 @@ #include "base/bind.h" #include "base/bind_helpers.h" #include "base/callback.h" +#include "base/logging.h" #include "base/memory/ref_counted.h" #include "base/memory/scoped_vector.h" #include "base/memory/weak_ptr.h" @@ -1432,125 +1433,77 @@ TEST_F(ClientSocketPoolBaseTest, CancelRequest) { EXPECT_EQ(ClientSocketPoolTest::kIndexOutOfBounds, GetOrderOfRequest(8)); } -class RequestSocketCallback : public TestCompletionCallbackBase { - public: - RequestSocketCallback(ClientSocketHandle* handle, - TestClientSocketPool* pool, - TestConnectJobFactory* test_connect_job_factory, - TestConnectJob::JobType next_job_type) - : handle_(handle), - pool_(pool), - within_callback_(false), - test_connect_job_factory_(test_connect_job_factory), - next_job_type_(next_job_type), - callback_(base::Bind(&RequestSocketCallback::OnComplete, - base::Unretained(this))) { - } - - ~RequestSocketCallback() override {} - - const CompletionCallback& callback() const { return callback_; } - - private: - void OnComplete(int result) { - SetResult(result); - ASSERT_EQ(OK, result); - - if (!within_callback_) { - test_connect_job_factory_->set_job_type(next_job_type_); - - // Don't allow reuse of the socket. Disconnect it and then release it and - // run through the MessageLoop once to get it completely released. - handle_->socket()->Disconnect(); - handle_->Reset(); - { - // TODO: Resolve conflicting intentions of stopping recursion with the - // |!within_callback_| test (above) and the call to |RunUntilIdle()| - // below. http://crbug.com/114130. - base::MessageLoop::ScopedNestableTaskAllower allow( - base::MessageLoop::current()); - base::MessageLoop::current()->RunUntilIdle(); - } - within_callback_ = true; - TestCompletionCallback next_job_callback; - scoped_refptr params( - new TestSocketParams(false /* ignore_limits */)); - int rv = handle_->Init("a", - params, - DEFAULT_PRIORITY, - next_job_callback.callback(), - pool_, - BoundNetLog()); - switch (next_job_type_) { - case TestConnectJob::kMockJob: - EXPECT_EQ(OK, rv); - break; - case TestConnectJob::kMockPendingJob: - EXPECT_EQ(ERR_IO_PENDING, rv); - - // For pending jobs, wait for new socket to be created. This makes - // sure there are no more pending operations nor any unclosed sockets - // when the test finishes. - // We need to give it a little bit of time to run, so that all the - // operations that happen on timers (e.g. cleanup of idle - // connections) can execute. - { - base::MessageLoop::ScopedNestableTaskAllower allow( - base::MessageLoop::current()); - base::PlatformThread::Sleep(base::TimeDelta::FromMilliseconds(10)); - EXPECT_EQ(OK, next_job_callback.WaitForResult()); - } - break; - default: - FAIL() << "Unexpected job type: " << next_job_type_; - break; - } - } +// Function to be used as a callback on socket request completion. It first +// disconnects the successfully connected socket from the first request, and +// then reuses the ClientSocketHandle to request another socket. +// +// |nested_callback| is called with the result of the second socket request. +void RequestSocketOnComplete(ClientSocketHandle* handle, + TestClientSocketPool* pool, + TestConnectJobFactory* test_connect_job_factory, + TestConnectJob::JobType next_job_type, + const CompletionCallback& nested_callback, + int first_request_result) { + EXPECT_EQ(OK, first_request_result); + + test_connect_job_factory->set_job_type(next_job_type); + + // Don't allow reuse of the socket. Disconnect it and then release it. + if (handle->socket()) + handle->socket()->Disconnect(); + handle->Reset(); + + scoped_refptr params( + new TestSocketParams(false /* ignore_limits */)); + TestCompletionCallback callback; + int rv = + handle->Init("a", params, LOWEST, nested_callback, pool, BoundNetLog()); + if (rv != ERR_IO_PENDING) { + DCHECK_EQ(TestConnectJob::kMockJob, next_job_type); + nested_callback.Run(rv); + } else { + DCHECK_EQ(TestConnectJob::kMockPendingJob, next_job_type); } +} - ClientSocketHandle* const handle_; - TestClientSocketPool* const pool_; - bool within_callback_; - TestConnectJobFactory* const test_connect_job_factory_; - TestConnectJob::JobType next_job_type_; - CompletionCallback callback_; -}; - +// Tests the case where a second socket is requested in a completion callback, +// and the second socket connects asynchronously. Reuses the same +// ClientSocketHandle for the second socket, after disconnecting the first. TEST_F(ClientSocketPoolBaseTest, RequestPendingJobTwice) { CreatePool(kDefaultMaxSockets, kDefaultMaxSocketsPerGroup); connect_job_factory_->set_job_type(TestConnectJob::kMockPendingJob); ClientSocketHandle handle; - RequestSocketCallback callback( - &handle, pool_.get(), connect_job_factory_, - TestConnectJob::kMockPendingJob); - int rv = handle.Init("a", - params_, - DEFAULT_PRIORITY, - callback.callback(), - pool_.get(), - BoundNetLog()); + TestCompletionCallback second_result_callback; + int rv = handle.Init( + "a", params_, DEFAULT_PRIORITY, + base::Bind(&RequestSocketOnComplete, &handle, pool_.get(), + connect_job_factory_, TestConnectJob::kMockPendingJob, + second_result_callback.callback()), + pool_.get(), BoundNetLog()); ASSERT_EQ(ERR_IO_PENDING, rv); - EXPECT_EQ(OK, callback.WaitForResult()); + EXPECT_EQ(OK, second_result_callback.WaitForResult()); } +// Tests the case where a second socket is requested in a completion callback, +// and the second socket connects synchronously. Reuses the same +// ClientSocketHandle for the second socket, after disconnecting the first. TEST_F(ClientSocketPoolBaseTest, RequestPendingJobThenSynchronous) { CreatePool(kDefaultMaxSockets, kDefaultMaxSocketsPerGroup); connect_job_factory_->set_job_type(TestConnectJob::kMockPendingJob); ClientSocketHandle handle; - RequestSocketCallback callback( - &handle, pool_.get(), connect_job_factory_, TestConnectJob::kMockJob); - int rv = handle.Init("a", - params_, - DEFAULT_PRIORITY, - callback.callback(), - pool_.get(), - BoundNetLog()); + TestCompletionCallback second_result_callback; + int rv = handle.Init( + "a", params_, DEFAULT_PRIORITY, + base::Bind(&RequestSocketOnComplete, &handle, pool_.get(), + connect_job_factory_, TestConnectJob::kMockPendingJob, + second_result_callback.callback()), + pool_.get(), BoundNetLog()); ASSERT_EQ(ERR_IO_PENDING, rv); - EXPECT_EQ(OK, callback.WaitForResult()); + EXPECT_EQ(OK, second_result_callback.WaitForResult()); } // Make sure that pending requests get serviced after active requests get diff --git a/net/socket/websocket_transport_client_socket_pool_unittest.cc b/net/socket/websocket_transport_client_socket_pool_unittest.cc index 2df660633168..47a255878b60 100644 --- a/net/socket/websocket_transport_client_socket_pool_unittest.cc +++ b/net/socket/websocket_transport_client_socket_pool_unittest.cc @@ -374,60 +374,38 @@ TEST_F(WebSocketTransportClientSocketPoolTest, CancelRequest) { EXPECT_EQ(ClientSocketPoolTest::kIndexOutOfBounds, GetOrderOfRequest(7)); } -class RequestSocketCallback : public TestCompletionCallbackBase { - public: - RequestSocketCallback(ClientSocketHandle* handle, - WebSocketTransportClientSocketPool* pool) - : handle_(handle), - pool_(pool), - within_callback_(false), - callback_(base::Bind(&RequestSocketCallback::OnComplete, - base::Unretained(this))) {} +// Function to be used as a callback on socket request completion. It first +// disconnects the successfully connected socket from the first request, and +// then reuses the ClientSocketHandle to request another socket. The second +// request is expected to succeed asynchronously. +// +// |nested_callback| is called with the result of the second socket request. +void RequestSocketOnComplete(ClientSocketHandle* handle, + WebSocketTransportClientSocketPool* pool, + const CompletionCallback& nested_callback, + int first_request_result) { + EXPECT_EQ(OK, first_request_result); + + // Don't allow reuse of the socket. Disconnect it and then release it. + handle->socket()->Disconnect(); + handle->Reset(); - ~RequestSocketCallback() override {} - - const CompletionCallback& callback() const { return callback_; } - - private: - void OnComplete(int result) { - SetResult(result); - ASSERT_EQ(OK, result); - - if (!within_callback_) { - // Don't allow reuse of the socket. Disconnect it and then release it and - // run through the MessageLoop once to get it completely released. - handle_->socket()->Disconnect(); - handle_->Reset(); - { - base::MessageLoop::ScopedNestableTaskAllower allow( - base::MessageLoop::current()); - base::MessageLoop::current()->RunUntilIdle(); - } - within_callback_ = true; - scoped_refptr dest( - new TransportSocketParams( - HostPortPair("www.google.com", 80), - false, - false, - OnHostResolutionCallback(), - TransportSocketParams::COMBINE_CONNECT_AND_WRITE_DEFAULT)); - int rv = - handle_->Init("a", dest, LOWEST, callback(), pool_, BoundNetLog()); - EXPECT_EQ(OK, rv); - } - } - - ClientSocketHandle* const handle_; - WebSocketTransportClientSocketPool* const pool_; - bool within_callback_; - CompletionCallback callback_; - - DISALLOW_COPY_AND_ASSIGN(RequestSocketCallback); -}; + scoped_refptr dest(new TransportSocketParams( + HostPortPair("www.google.com", 80), false, false, + OnHostResolutionCallback(), + TransportSocketParams::COMBINE_CONNECT_AND_WRITE_DEFAULT)); + int rv = + handle->Init("a", dest, LOWEST, nested_callback, pool, BoundNetLog()); + EXPECT_EQ(ERR_IO_PENDING, rv); + if (ERR_IO_PENDING != rv) + nested_callback.Run(rv); +} +// Tests the case where a second socket is requested in a completion callback, +// and the second socket connects asynchronously. Reuses the same +// ClientSocketHandle for the second socket, after disconnecting the first. TEST_F(WebSocketTransportClientSocketPoolTest, RequestTwice) { ClientSocketHandle handle; - RequestSocketCallback callback(&handle, &pool_); scoped_refptr dest( new TransportSocketParams( HostPortPair("www.google.com", 80), @@ -435,15 +413,13 @@ TEST_F(WebSocketTransportClientSocketPoolTest, RequestTwice) { false, OnHostResolutionCallback(), TransportSocketParams::COMBINE_CONNECT_AND_WRITE_DEFAULT)); - int rv = handle.Init( - "a", dest, LOWEST, callback.callback(), &pool_, BoundNetLog()); + TestCompletionCallback second_result_callback; + int rv = handle.Init("a", dest, LOWEST, + base::Bind(&RequestSocketOnComplete, &handle, &pool_, + second_result_callback.callback()), + &pool_, BoundNetLog()); ASSERT_EQ(ERR_IO_PENDING, rv); - - // The callback is going to request "www.google.com". We want it to complete - // synchronously this time. - host_resolver_->set_synchronous_mode(true); - - EXPECT_EQ(OK, callback.WaitForResult()); + EXPECT_EQ(OK, second_result_callback.WaitForResult()); handle.Reset(); } -- 2.11.4.GIT