summaryrefslogtreecommitdiffstats
path: root/net
diff options
context:
space:
mode:
authorwillchan@chromium.org <willchan@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98>2010-01-13 18:44:11 +0000
committerwillchan@chromium.org <willchan@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98>2010-01-13 18:44:11 +0000
commit5edbf8d2dd9f058f00c14c1f485113247c6673a5 (patch)
tree2cc8179493455b7f39555bda8df712a8008985fc /net
parentd5e44cf22db4db096314add16b66f183892fab13 (diff)
downloadchromium_src-5edbf8d2dd9f058f00c14c1f485113247c6673a5.zip
chromium_src-5edbf8d2dd9f058f00c14c1f485113247c6673a5.tar.gz
chromium_src-5edbf8d2dd9f058f00c14c1f485113247c6673a5.tar.bz2
Slightly tweak socket late binding behavior to make it more deterministic which is useful for tests.
When a socket has been released to ClientSocketPoolBase, it is not immediately released, but a DoReleaseSocket() task gets posted to the MessageLoop. In many of our tests, what can happen is right after ClientSocketHandle::Reset() gets called, which posts the aforementioned release task, another ClientSocketHandle requests a socket, which starts up a ConnectJob that will completely asynchronously, but AFTER the release task happens. Note that if the released socket is no longer connected, then when DoReleaseSocket() completes, it will try to process a pending request, thereby initiating an extra ConnectJob. So, even though ClientSocketHandle::Init() may only have been called twice, we can see 3 ConnectJobs, which is weird. It's mostly harmless since this race condition probably doesn't happen much in real use, and the only side effect is an extra ConnectJob which might even be faster, but this behavior introduces difficult to control behavior in tests. The solution taken here is to keep track of the number of pending released sockets for a ClientSocketPool group. If there are any pending released sockets, then don't start up a new ConnectJob yet, wait for the sockets to get released. This can introduce some delays for starting up ConnectJobs, but the delay should be very small (one MessageLoop iteration through the IO thread should not take long) and it only happens in the edge case where a new ClientSocketPool request comes in for the same group where a DoReleaseSocket() task has been posted. In these cases, we may also prevent unnecessary ConnectJobs from getting started up, especially if the socket is released and able to be reused. Existing unit tests (ClientSocketPoolBaseTest_LateBinding_RequestPendingJob*) cover this change in behavior. Review URL: http://codereview.chromium.org/550024 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@36144 0039d316-1c4b-4281-b951-d872f2087c98
Diffstat (limited to 'net')
-rw-r--r--net/socket/client_socket_pool_base.cc16
-rw-r--r--net/socket/client_socket_pool_base.h14
-rw-r--r--net/socket/client_socket_pool_base_unittest.cc22
3 files changed, 43 insertions, 9 deletions
diff --git a/net/socket/client_socket_pool_base.cc b/net/socket/client_socket_pool_base.cc
index 44e2187..037c9ff 100644
--- a/net/socket/client_socket_pool_base.cc
+++ b/net/socket/client_socket_pool_base.cc
@@ -190,6 +190,7 @@ int ClientSocketPoolBaseHelper::RequestSocket(
return ERR_IO_PENDING;
}
+ // Try to reuse a socket.
while (!group.idle_sockets.empty()) {
IdleSocket idle_socket = group.idle_sockets.back();
group.idle_sockets.pop_back();
@@ -205,6 +206,13 @@ int ClientSocketPoolBaseHelper::RequestSocket(
delete idle_socket.socket;
}
+ // See if we already have enough connect jobs or sockets that will be released
+ // soon.
+ if (g_late_binding && group.HasReleasingSockets()) {
+ InsertRequestIntoQueue(request, &group.pending_requests);
+ return ERR_IO_PENDING;
+ }
+
// We couldn't find a socket to reuse, so allocate and connect a new one.
// If we aren't using late binding, the job lines up with a request so
@@ -214,7 +222,7 @@ int ClientSocketPoolBaseHelper::RequestSocket(
scoped_ptr<ConnectJob> connect_job(
connect_job_factory_->NewConnectJob(group_name, *request, this,
- job_load_log));
+ job_load_log));
int rv = connect_job->Connect();
@@ -285,6 +293,9 @@ void ClientSocketPoolBaseHelper::CancelRequest(
void ClientSocketPoolBaseHelper::ReleaseSocket(const std::string& group_name,
ClientSocket* socket) {
+ Group& group = group_map_[group_name];
+ group.num_releasing_sockets++;
+ DCHECK_LE(group.num_releasing_sockets, group.active_socket_count);
// Run this asynchronously to allow the caller to finish before we let
// another to begin doing work. This also avoids nasty recursion issues.
// NOTE: We cannot refer to the handle argument after this method returns.
@@ -410,6 +421,9 @@ void ClientSocketPoolBaseHelper::DoReleaseSocket(const std::string& group_name,
Group& group = i->second;
+ group.num_releasing_sockets--;
+ DCHECK_GE(group.num_releasing_sockets, 0);
+
CHECK(handed_out_socket_count_ > 0);
handed_out_socket_count_--;
diff --git a/net/socket/client_socket_pool_base.h b/net/socket/client_socket_pool_base.h
index b21bfb8..8132065 100644
--- a/net/socket/client_socket_pool_base.h
+++ b/net/socket/client_socket_pool_base.h
@@ -256,8 +256,14 @@ class ClientSocketPoolBaseHelper
// A Group is allocated per group_name when there are idle sockets or pending
// requests. Otherwise, the Group object is removed from the map.
+ // |active_socket_count| tracks the number of sockets held by clients. Of
+ // this number of sockets held by clients, some of them may be released soon,
+ // since ReleaseSocket() was called of them, but the DoReleaseSocket() task
+ // has not run yet for them. |num_releasing_sockets| tracks these values,
+ // which is useful for not starting up new ConnectJobs when sockets may become
+ // available really soon.
struct Group {
- Group() : active_socket_count(0) {}
+ Group() : active_socket_count(0), num_releasing_sockets(0) {}
bool IsEmpty() const {
return active_socket_count == 0 && idle_sockets.empty() && jobs.empty() &&
@@ -269,6 +275,10 @@ class ClientSocketPoolBaseHelper
max_sockets_per_group;
}
+ bool HasReleasingSockets() const {
+ return num_releasing_sockets > 0;
+ }
+
RequestPriority TopPendingPriority() const {
return pending_requests.front()->priority();
}
@@ -278,6 +288,8 @@ class ClientSocketPoolBaseHelper
RequestQueue pending_requests;
RequestMap connecting_requests;
int active_socket_count; // number of active sockets used by clients
+ // Number of sockets being released within one loop through the MessageLoop.
+ int num_releasing_sockets;
};
typedef std::map<std::string, Group> GroupMap;
diff --git a/net/socket/client_socket_pool_base_unittest.cc b/net/socket/client_socket_pool_base_unittest.cc
index db61140..6f3f5ef 100644
--- a/net/socket/client_socket_pool_base_unittest.cc
+++ b/net/socket/client_socket_pool_base_unittest.cc
@@ -981,7 +981,16 @@ class RequestSocketCallback : public CallbackRunner< Tuple1<int> > {
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();
+ {
+ MessageLoop::ScopedNestableTaskAllower nestable(
+ MessageLoop::current());
+ MessageLoop::current()->RunAllPending();
+ }
within_callback_ = true;
TestCompletionCallback next_job_callback;
int rv = InitHandle(
@@ -1000,9 +1009,12 @@ class RequestSocketCallback : public CallbackRunner< Tuple1<int> > {
// 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.
- MessageLoop::current()->SetNestableTasksAllowed(true);
- PlatformThread::Sleep(10);
- EXPECT_EQ(OK, next_job_callback.WaitForResult());
+ {
+ MessageLoop::ScopedNestableTaskAllower nestable(
+ MessageLoop::current());
+ PlatformThread::Sleep(10);
+ EXPECT_EQ(OK, next_job_callback.WaitForResult());
+ }
break;
default:
FAIL() << "Unexpected job type: " << next_job_type_;
@@ -1037,7 +1049,6 @@ TEST_F(ClientSocketPoolBaseTest, RequestPendingJobTwice) {
ASSERT_EQ(ERR_IO_PENDING, rv);
EXPECT_EQ(OK, callback.WaitForResult());
- handle.Reset();
}
TEST_F(ClientSocketPoolBaseTest, RequestPendingJobThenSynchronous) {
@@ -1052,7 +1063,6 @@ TEST_F(ClientSocketPoolBaseTest, RequestPendingJobThenSynchronous) {
ASSERT_EQ(ERR_IO_PENDING, rv);
EXPECT_EQ(OK, callback.WaitForResult());
- handle.Reset();
}
// Make sure that pending requests get serviced after active requests get
@@ -1598,7 +1608,6 @@ TEST_F(ClientSocketPoolBaseTest_LateBinding, RequestPendingJobTwice) {
ASSERT_EQ(ERR_IO_PENDING, rv);
EXPECT_EQ(OK, callback.WaitForResult());
- handle.Reset();
}
TEST_F(ClientSocketPoolBaseTest_LateBinding, RequestPendingJobThenSynchronous) {
@@ -1614,7 +1623,6 @@ TEST_F(ClientSocketPoolBaseTest_LateBinding, RequestPendingJobThenSynchronous) {
ASSERT_EQ(ERR_IO_PENDING, rv);
EXPECT_EQ(OK, callback.WaitForResult());
- handle.Reset();
}
// Make sure that pending requests get serviced after active requests get