shill: Fix timer canceling
The move to new callback closures included the frequent replacement
of "task_factory_.RevokeAll()" with
"weak_ptr_factory_.InvalidateWeakPtrs()". The former canceled all
pending timers, whereas the latter also effectively canceled all
I/O callbacks. This caused both portal detection and the service
proxy to stop working.
This CL creates CancelableClosures for the affected timers, and
cancels these closures instead of invalidating all weak pointers.
BUG=chromium-os:28885
TEST=Manual: Test both passing and failure cases of portal detection,
ensuring that DNS timeouts work correctly. Use "curl -x" to test
HTTP proxy.
Change-Id: Id61dfb6a1a4ce0defaa08a99e318bc510c6c84b3
Reviewed-on: https://gerrit.chromium.org/gerrit/19644
Reviewed-by: Eric Shienbrood <ers@chromium.org>
Commit-Ready: Paul Stewart <pstew@chromium.org>
Tested-by: Paul Stewart <pstew@chromium.org>
diff --git a/dns_client.cc b/dns_client.cc
index b357e02..19cf6bc 100644
--- a/dns_client.cc
+++ b/dns_client.cc
@@ -67,10 +67,6 @@
running_(false),
resolver_state_(NULL),
weak_ptr_factory_(this),
- read_callback_(Bind(&DNSClient::HandleDNSRead,
- weak_ptr_factory_.GetWeakPtr())),
- write_callback_(Bind(&DNSClient::HandleDNSWrite,
- weak_ptr_factory_.GetWeakPtr())),
ares_(Ares::GetInstance()),
time_(Time::GetInstance()) {}
@@ -197,7 +193,7 @@
}
VLOG(3) << "In " << __func__;
running_ = false;
- weak_ptr_factory_.InvalidateWeakPtrs();
+ timeout_closure_.Cancel();
dispatcher_->PostTask(Bind(&DNSClient::HandleCompletion,
weak_ptr_factory_.GetWeakPtr()));
@@ -275,6 +271,10 @@
int action_bits = ares_->GetSock(resolver_state_->channel, sockets,
ARES_GETSOCK_MAXNUM);
+ base::Callback<void(int)> read_callback(
+ Bind(&DNSClient::HandleDNSRead, weak_ptr_factory_.GetWeakPtr()));
+ base::Callback<void(int)> write_callback(
+ Bind(&DNSClient::HandleDNSWrite, weak_ptr_factory_.GetWeakPtr()));
for (int i = 0; i < ARES_GETSOCK_MAXNUM; i++) {
if (ARES_GETSOCK_READABLE(action_bits, i)) {
if (ContainsKey(old_read, sockets[i])) {
@@ -284,7 +284,7 @@
std::tr1::shared_ptr<IOHandler> (
dispatcher_->CreateReadyHandler(sockets[i],
IOHandler::kModeInput,
- read_callback_));
+ read_callback));
}
}
if (ARES_GETSOCK_WRITABLE(action_bits, i)) {
@@ -295,7 +295,7 @@
std::tr1::shared_ptr<IOHandler> (
dispatcher_->CreateReadyHandler(sockets[i],
IOHandler::kModeOutput,
- write_callback_));
+ write_callback));
}
}
}
@@ -313,7 +313,7 @@
timersub(&now, &resolver_state_->start_time_, &elapsed_time);
timeout_tv.tv_sec = timeout_ms_ / 1000;
timeout_tv.tv_usec = (timeout_ms_ % 1000) * 1000;
- weak_ptr_factory_.InvalidateWeakPtrs();
+ timeout_closure_.Cancel();
if (timercmp(&elapsed_time, &timeout_tv, >=)) {
// There are 3 cases of interest:
@@ -336,9 +336,10 @@
timersub(&timeout_tv, &elapsed_time, &max);
struct timeval *tv = ares_->Timeout(resolver_state_->channel,
&max, &ret_tv);
- dispatcher_->PostDelayedTask(
- Bind(&DNSClient::HandleTimeout, weak_ptr_factory_.GetWeakPtr()),
- tv->tv_sec * 1000 + tv->tv_usec / 1000);
+ timeout_closure_.Reset(
+ Bind(&DNSClient::HandleTimeout, weak_ptr_factory_.GetWeakPtr()));
+ dispatcher_->PostDelayedTask(timeout_closure_.callback(),
+ tv->tv_sec * 1000 + tv->tv_usec / 1000);
}
return true;
diff --git a/dns_client.h b/dns_client.h
index b49a5c9..bd2d1d4 100644
--- a/dns_client.h
+++ b/dns_client.h
@@ -9,6 +9,7 @@
#include <vector>
#include <base/callback.h>
+#include <base/cancelable_callback.h>
#include <base/memory/scoped_ptr.h>
#include <base/memory/weak_ptr.h>
#include <gtest/gtest_prod.h> // for FRIEND_TEST
@@ -83,9 +84,8 @@
int timeout_ms_;
bool running_;
scoped_ptr<DNSClientState> resolver_state_;
+ base::CancelableClosure timeout_closure_;
base::WeakPtrFactory<DNSClient> weak_ptr_factory_;
- base::Callback<void(int)> read_callback_;
- base::Callback<void(int)> write_callback_;
Ares *ares_;
Time *time_;
diff --git a/dns_client_unittest.cc b/dns_client_unittest.cc
index 5097756..713487b 100644
--- a/dns_client_unittest.cc
+++ b/dns_client_unittest.cc
@@ -205,8 +205,6 @@
void ExpectReset() {
EXPECT_TRUE(dns_client_->address_.family() == IPAddress::kFamilyIPv4);
EXPECT_TRUE(dns_client_->address_.IsDefault());
- EXPECT_FALSE(dns_client_->read_callback_.is_null());
- EXPECT_FALSE(dns_client_->write_callback_.is_null());
EXPECT_FALSE(dns_client_->resolver_state_.get());
}
diff --git a/http_proxy.cc b/http_proxy.cc
index 3d2fd0c..d4b31fd 100644
--- a/http_proxy.cc
+++ b/http_proxy.cc
@@ -178,8 +178,9 @@
dispatcher_->CreateInputHandler(client_socket_,
read_client_callback_));
// Overall transaction timeout.
- dispatcher_->PostDelayedTask(Bind(&HTTPProxy::StopClient,
- weak_ptr_factory_.GetWeakPtr()),
+ transaction_timeout_.Reset(Bind(&HTTPProxy::StopClient,
+ weak_ptr_factory_.GetWeakPtr()));
+ dispatcher_->PostDelayedTask(transaction_timeout_.callback(),
kTransactionTimeoutSeconds * 1000);
state_ = kStateReadClientHeader;
@@ -646,7 +647,7 @@
dns_client_->Stop();
server_async_connection_->Stop();
idle_timeout_.Cancel();
- weak_ptr_factory_.InvalidateWeakPtrs();
+ transaction_timeout_.Cancel();
accept_handler_->Start();
state_ = kStateWaitConnection;
}
diff --git a/http_proxy.h b/http_proxy.h
index 1a88d89..3303266 100644
--- a/http_proxy.h
+++ b/http_proxy.h
@@ -144,6 +144,7 @@
int server_socket_;
bool is_route_requested_;
base::CancelableClosure idle_timeout_;
+ base::CancelableClosure transaction_timeout_;
std::vector<std::string> client_headers_;
std::string server_hostname_;
ByteString client_data_;
diff --git a/http_request.cc b/http_request.cc
index b718831..bf22225 100644
--- a/http_request.cc
+++ b/http_request.cc
@@ -140,7 +140,7 @@
sockets_->Close(server_socket_);
server_socket_ = -1;
}
- weak_ptr_factory_.InvalidateWeakPtrs();
+ timeout_closure_.Cancel();
timeout_result_ = kResultUnknown;
}
@@ -229,10 +229,10 @@
// Start a timeout for "the next event".
void HTTPRequest::StartIdleTimeout(int timeout_seconds, Result timeout_result) {
timeout_result_ = timeout_result;
- weak_ptr_factory_.InvalidateWeakPtrs();
- dispatcher_->PostDelayedTask(
- Bind(&HTTPRequest::TimeoutTask, weak_ptr_factory_.GetWeakPtr()),
- timeout_seconds * 1000);
+ timeout_closure_.Reset(
+ Bind(&HTTPRequest::TimeoutTask, weak_ptr_factory_.GetWeakPtr()));
+ dispatcher_->PostDelayedTask(timeout_closure_.callback(),
+ timeout_seconds * 1000);
}
void HTTPRequest::TimeoutTask() {
diff --git a/http_request.h b/http_request.h
index ed3fbbb..610ef0d 100644
--- a/http_request.h
+++ b/http_request.h
@@ -9,6 +9,7 @@
#include <vector>
#include <base/callback.h>
+#include <base/cancelable_callback.h>
#include <base/memory/ref_counted.h>
#include <base/memory/scoped_ptr.h>
#include <base/memory/weak_ptr.h>
@@ -121,6 +122,7 @@
std::string server_hostname_;
int server_port_;
int server_socket_;
+ base::CancelableClosure timeout_closure_;
Result timeout_result_;
ByteString request_data_;
ByteString response_data_;
diff --git a/portal_detector.cc b/portal_detector.cc
index 2072efa..65767f6 100644
--- a/portal_detector.cc
+++ b/portal_detector.cc
@@ -96,6 +96,7 @@
return;
}
+ start_attempt_.Cancel();
StopAttempt();
attempt_count_ = 0;
request_.reset();
@@ -226,9 +227,10 @@
} else {
next_attempt_delay = init_delay_seconds * 1000;
}
- dispatcher_->PostDelayedTask(
- Bind(&PortalDetector::StartAttemptTask, weak_ptr_factory_.GetWeakPtr()),
- next_attempt_delay);
+
+ start_attempt_.Reset(Bind(&PortalDetector::StartAttemptTask,
+ weak_ptr_factory_.GetWeakPtr()));
+ dispatcher_->PostDelayedTask(start_attempt_.callback(), next_attempt_delay);
}
void PortalDetector::StartAttemptTask() {
@@ -245,14 +247,15 @@
return;
}
- dispatcher_->PostDelayedTask(
- Bind(&PortalDetector::TimeoutAttemptTask, weak_ptr_factory_.GetWeakPtr()),
- kRequestTimeoutSeconds * 1000);
+ attempt_timeout_.Reset(Bind(&PortalDetector::TimeoutAttemptTask,
+ weak_ptr_factory_.GetWeakPtr()));
+ dispatcher_->PostDelayedTask(attempt_timeout_.callback(),
+ kRequestTimeoutSeconds * 1000);
}
void PortalDetector::StopAttempt() {
request_->Stop();
- weak_ptr_factory_.InvalidateWeakPtrs();
+ attempt_timeout_.Cancel();
}
void PortalDetector::TimeoutAttemptTask() {
diff --git a/portal_detector.h b/portal_detector.h
index 2bc381d..802e49c 100644
--- a/portal_detector.h
+++ b/portal_detector.h
@@ -9,6 +9,7 @@
#include <vector>
#include <base/callback.h>
+#include <base/cancelable_callback.h>
#include <base/memory/ref_counted.h>
#include <base/memory/scoped_ptr.h>
#include <base/memory/weak_ptr.h>
@@ -154,6 +155,8 @@
ConnectionRefPtr connection_;
EventDispatcher *dispatcher_;
base::WeakPtrFactory<PortalDetector> weak_ptr_factory_;
+ base::CancelableClosure attempt_timeout_;
+ base::CancelableClosure start_attempt_;
base::Callback<void(const Result &)> portal_result_callback_;
scoped_ptr<HTTPRequest> request_;
base::Callback<void(const ByteString &)> request_read_callback_;