shill: Prepare DNSClient and HTTPRequest for destroy-on-callback
Refactor both classes so that it's okay for the callback to destroy
the object (i.e., do nothing to the object after the callback is
called). As a part of this, clean up some of the callback semantics,
so, for example, DNSClient callbacks are passed an Error reference
and an IP Address instead of having to use a getter. Additionally
remove the blemish where an immediate timeout in Start() both returned
failure and called the callback.
BUG=chromium-os:23318
TEST=Fixed unit tests, manual
Change-Id: Ib7787a7aa6f7f3d00caa539d6b0221ff5f3d60b3
Reviewed-on: https://gerrit.chromium.org/gerrit/16435
Commit-Ready: Paul Stewart <pstew@chromium.org>
Reviewed-by: Paul Stewart <pstew@chromium.org>
Tested-by: Paul Stewart <pstew@chromium.org>
diff --git a/dns_client.cc b/dns_client.cc
index fed71b2..cbadb1e 100644
--- a/dns_client.cc
+++ b/dns_client.cc
@@ -16,6 +16,7 @@
#include <vector>
#include <base/stl_util-inl.h>
+#include <base/string_number_conversions.h>
#include "shill/shill_ares.h"
#include "shill/shill_time.h"
@@ -52,7 +53,7 @@
const vector<string> &dns_servers,
int timeout_ms,
EventDispatcher *dispatcher,
- Callback1<bool>::Type *callback)
+ ClientCallback *callback)
: address_(IPAddress(family)),
interface_name_(interface_name),
dns_servers_(dns_servers),
@@ -71,9 +72,10 @@
Stop();
}
-bool DNSClient::Start(const string &hostname) {
+bool DNSClient::Start(const string &hostname, Error *error) {
if (running_) {
- LOG(ERROR) << "Only one DNS request is allowed at a time";
+ Error::PopulateAndLog(error, Error::kInProgress,
+ "Only one DNS request is allowed at a time");
return false;
}
@@ -92,7 +94,8 @@
}
if (server_addresses.empty()) {
- LOG(ERROR) << "No valid DNS server addresses";
+ Error::PopulateAndLog(error, Error::kInvalidArguments,
+ "No valid DNS server addresses");
return false;
}
@@ -105,7 +108,9 @@
&options,
ARES_OPT_SERVERS | ARES_OPT_TIMEOUTMS);
if (status != ARES_SUCCESS) {
- LOG(ERROR) << "ARES initialization returns error code: " << status;
+ Error::PopulateAndLog(error, Error::kOperationFailed,
+ "ARES initialization returns error code: " +
+ base::IntToString(status));
resolver_state_.reset();
return false;
}
@@ -115,12 +120,12 @@
running_ = true;
time_->GetTimeMonotonic(&resolver_state_->start_time_);
- error_.clear();
ares_->GetHostByName(resolver_state_->channel, hostname.c_str(),
address_.family(), ReceiveDNSReplyCB, this);
if (!RefreshHandles()) {
LOG(ERROR) << "Impossibly short timeout.";
+ error->CopyFrom(error_);
Stop();
return false;
}
@@ -129,16 +134,41 @@
}
void DNSClient::Stop() {
+ VLOG(3) << "In " << __func__;
if (!resolver_state_.get()) {
return;
}
running_ = false;
task_factory_.RevokeAll();
+ error_.Reset();
+ address_.SetAddressToDefault();
ares_->Destroy(resolver_state_->channel);
resolver_state_.reset();
}
+// We delay our call to completion so that we exit all IOHandlers, and
+// can clean up all of our local state before calling the callback, or
+// during the process of the execution of the callee (which is free to
+// call our destructor safely).
+void DNSClient::HandleCompletion() {
+ VLOG(3) << "In " << __func__;
+ Error error;
+ error.CopyFrom(error_);
+ IPAddress address(address_);
+ if (!error.IsSuccess()) {
+ // If the DNS request did not succeed, do not trust it for future
+ // attempts.
+ Stop();
+ } else {
+ // Prepare our state for the next request without destroying the
+ // current ARES state.
+ error_.Reset();
+ address_.SetAddressToDefault();
+ }
+ callback_->Run(error, address);
+}
+
void DNSClient::HandleDNSRead(int fd) {
ares_->ProcessFd(resolver_state_->channel, fd, ARES_SOCKET_BAD);
RefreshHandles();
@@ -151,19 +181,7 @@
void DNSClient::HandleTimeout() {
ares_->ProcessFd(resolver_state_->channel, ARES_SOCKET_BAD, ARES_SOCKET_BAD);
- if (!RefreshHandles()) {
- // If we have timed out, ARES might still have sockets open.
- // Force them closed by doing an explicit shutdown. This is
- // different from HandleDNSRead and HandleDNSWrite where any
- // change in our running_ state would be as a result of ARES
- // itself and therefore properly synchronized with it: if a
- // search completes during the course of ares_->ProcessFd(),
- // the ARES fds and other state is guaranteed to have cleaned
- // up and ready for a new request. Since this timeout is
- // genererated outside of the library it is best to completely
- // shutdown ARES and start with fresh state for a new request.
- Stop();
- }
+ RefreshHandles();
}
void DNSClient::ReceiveDNSReply(int status, struct hostent *hostent) {
@@ -171,7 +189,11 @@
// We can be called during ARES shutdown -- ignore these events.
return;
}
+ VLOG(3) << "In " << __func__;
running_ = false;
+ task_factory_.RevokeAll();
+ dispatcher_->PostTask(task_factory_.NewRunnableMethod(
+ &DNSClient::HandleCompletion));
if (status == ARES_SUCCESS &&
hostent != NULL &&
@@ -182,41 +204,40 @@
address_ = IPAddress(address_.family(),
ByteString(reinterpret_cast<unsigned char *>(
hostent->h_addr_list[0]), hostent->h_length));
- callback_->Run(true);
} else {
switch (status) {
case ARES_ENODATA:
- error_ = kErrorNoData;
+ error_.Populate(Error::kOperationFailed, kErrorNoData);
break;
case ARES_EFORMERR:
- error_ = kErrorFormErr;
+ error_.Populate(Error::kOperationFailed, kErrorFormErr);
break;
case ARES_ESERVFAIL:
- error_ = kErrorServerFail;
+ error_.Populate(Error::kOperationFailed, kErrorServerFail);
break;
case ARES_ENOTFOUND:
- error_ = kErrorNotFound;
+ error_.Populate(Error::kOperationFailed, kErrorNotFound);
break;
case ARES_ENOTIMP:
- error_ = kErrorNotImp;
+ error_.Populate(Error::kOperationFailed, kErrorNotImp);
break;
case ARES_EREFUSED:
- error_ = kErrorRefused;
+ error_.Populate(Error::kOperationFailed, kErrorRefused);
break;
case ARES_EBADQUERY:
case ARES_EBADNAME:
case ARES_EBADFAMILY:
case ARES_EBADRESP:
- error_ = kErrorBadQuery;
+ error_.Populate(Error::kOperationFailed, kErrorBadQuery);
break;
case ARES_ECONNREFUSED:
- error_ = kErrorNetRefused;
+ error_.Populate(Error::kOperationFailed, kErrorNetRefused);
break;
case ARES_ETIMEOUT:
- error_ = kErrorTimedOut;
+ error_.Populate(Error::kOperationTimeout, kErrorTimedOut);
break;
default:
- error_ = kErrorUnknown;
+ error_.Populate(Error::kOperationFailed, kErrorUnknown);
if (status == ARES_SUCCESS) {
LOG(ERROR) << "ARES returned success but hostent was invalid!";
} else {
@@ -224,7 +245,6 @@
}
break;
}
- callback_->Run(false);
}
}
@@ -246,7 +266,7 @@
ares_socket_t sockets[ARES_GETSOCK_MAXNUM];
int action_bits = ares_->GetSock(resolver_state_->channel, sockets,
- ARES_GETSOCK_MAXNUM);
+ ARES_GETSOCK_MAXNUM);
for (int i = 0; i < ARES_GETSOCK_MAXNUM; i++) {
if (ARES_GETSOCK_READABLE(action_bits, i)) {
@@ -274,9 +294,8 @@
}
if (!running_) {
- // We are here just to clean up socket and timer handles, and the
- // ARES state was cleaned up during the last call to ares_process_fd().
- task_factory_.RevokeAll();
+ // We are here just to clean up socket handles, and the ARES state was
+ // cleaned up during the last call to ares_->ProcessFd().
return false;
}
@@ -287,28 +306,29 @@
timersub(&now, &resolver_state_->start_time_, &elapsed_time);
timeout_tv.tv_sec = timeout_ms_ / 1000;
timeout_tv.tv_usec = (timeout_ms_ % 1000) * 1000;
+ task_factory_.RevokeAll();
+
if (timercmp(&elapsed_time, &timeout_tv, >=)) {
// There are 3 cases of interest:
- // - If we got here from Start(), we will have the side-effect of
- // both invoking the callback and returning False in Start().
- // Start() will call Stop() which will shut down ARES.
- // - If we got here from the tail of an IO event (racing with the
- // timer, we can't call Stop() since that will blow away the
- // IOHandler we are running in, however we will soon be called
- // again by the timeout proc so we can clean up the ARES state
- // then.
- // - If we got here from a timeout handler, it will safely call
- // Stop() when we return false.
- error_ = kErrorTimedOut;
- callback_->Run(false);
+ // - If we got here from Start(), when we return, Stop() will be
+ // called, so our cleanup task will not run, so we will not have the
+ // side-effect of both invoking the callback and returning False
+ // in Start().
+ // - If we got here from the tail of an IO event, we can't call
+ // Stop() since that will blow away the IOHandler we are running
+ // in. We will perform the cleanup in the posted task below.
+ // - If we got here from a timeout handler, we will perform cleanup
+ // in the posted task.
running_ = false;
+ error_.Populate(Error::kOperationTimeout, kErrorTimedOut);
+ dispatcher_->PostTask(task_factory_.NewRunnableMethod(
+ &DNSClient::HandleCompletion));
return false;
} else {
struct timeval max, ret_tv;
timersub(&timeout_tv, &elapsed_time, &max);
struct timeval *tv = ares_->Timeout(resolver_state_->channel,
&max, &ret_tv);
- task_factory_.RevokeAll();
dispatcher_->PostDelayedTask(
task_factory_.NewRunnableMethod(&DNSClient::HandleTimeout),
tv->tv_sec * 1000 + tv->tv_usec / 1000);