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);
diff --git a/dns_client.h b/dns_client.h
index 9f13c20..735aca1 100644
--- a/dns_client.h
+++ b/dns_client.h
@@ -13,6 +13,7 @@
#include <base/task.h>
#include <gtest/gtest_prod.h> // for FRIEND_TEST
+#include "shill/error.h"
#include "shill/event_dispatcher.h"
#include "shill/ip_address.h"
#include "shill/refptr_types.h"
@@ -25,9 +26,11 @@
class Time;
struct DNSClientState;
-// Implements a DNS resolution client that can run asynchronously
+// Implements a DNS resolution client that can run asynchronously.
class DNSClient {
public:
+ typedef Callback2<const Error &, const IPAddress &>::Type ClientCallback;
+
static const int kDefaultTimeoutMS;
static const char kErrorNoData[];
static const char kErrorFormErr[];
@@ -45,17 +48,24 @@
const std::vector<std::string> &dns_servers,
int timeout_ms,
EventDispatcher *dispatcher,
- Callback1<bool>::Type *callback);
+ ClientCallback *callback);
virtual ~DNSClient();
- virtual bool Start(const std::string &hostname);
+ // Returns true if the DNS client started successfully, false otherwise.
+ // If successful, the callback will be called with the result of the
+ // request. If Start() fails and returns false, the callback will not
+ // be called, but the error that caused the failure will be returned in
+ // |error|.
+ virtual bool Start(const std::string &hostname, Error *error);
+
+ // Aborts any running DNS client transaction. This will cancel any callback
+ // invocation.
virtual void Stop();
- virtual const IPAddress &address() const { return address_; }
- virtual const std::string &error() const { return error_; }
private:
friend class DNSClientTest;
+ void HandleCompletion();
void HandleDNSRead(int fd);
void HandleDNSWrite(int fd);
void HandleTimeout();
@@ -64,14 +74,14 @@
struct hostent *hostent);
bool RefreshHandles();
+ Error error_;
IPAddress address_;
std::string interface_name_;
std::vector<std::string> dns_servers_;
EventDispatcher *dispatcher_;
- Callback1<bool>::Type *callback_;
+ ClientCallback *callback_;
int timeout_ms_;
bool running_;
- std::string error_;
scoped_ptr<DNSClientState> resolver_state_;
scoped_ptr<Callback1<int>::Type> read_callback_;
scoped_ptr<Callback1<int>::Type> write_callback_;
diff --git a/dns_client_unittest.cc b/dns_client_unittest.cc
index 720240e..33e87d3 100644
--- a/dns_client_unittest.cc
+++ b/dns_client_unittest.cc
@@ -13,6 +13,7 @@
#include <gtest/gtest.h>
#include <gmock/gmock.h>
+#include "shill/error.h"
#include "shill/event_dispatcher.h"
#include "shill/io_handler.h"
#include "shill/mock_ares.h"
@@ -50,9 +51,18 @@
const int kAresWaitMS = 1000; // Time period ARES asks caller to wait
} // namespace {}
+MATCHER_P(IsSuccess, is_success, "") {
+ return is_success == arg.IsSuccess();
+}
+
+MATCHER_P2(IsFailure, failure_type, failure_message, "") {
+ return failure_type == arg.type() && failure_message == arg.message();
+}
+
class DNSClientTest : public Test {
public:
- DNSClientTest() : ares_result_(ARES_SUCCESS) {
+ DNSClientTest()
+ : ares_result_(ARES_SUCCESS), address_result_(IPAddress::kFamilyUnknown) {
time_val_.tv_sec = 0;
time_val_.tv_usec = 0;
ares_timeout_.tv_sec = kAresWaitMS / 1000;
@@ -100,6 +110,10 @@
dns_client_->HandleTimeout();
}
+ void CallCompletion() {
+ dns_client_->HandleCompletion();
+ }
+
void CreateClient(const vector<string> &dns_servers, int timeout_ms) {
dns_client_.reset(new DNSClient(IPAddress::kFamilyIPv4,
kNetworkInterface,
@@ -147,19 +161,49 @@
.WillOnce(ReturnNew<IOHandler>());
SetActive();
EXPECT_CALL(dispatcher_, PostDelayedTask(_, kAresWaitMS));
- ASSERT_TRUE(dns_client_->Start(kGoodName));
+ Error error;
+ ASSERT_TRUE(dns_client_->Start(kGoodName, &error));
+ EXPECT_TRUE(error.IsSuccess());
EXPECT_CALL(ares_, Destroy(kAresChannel));
}
void TestValidCompletion() {
- EXPECT_CALL(callback_target_, CallTarget(true));
EXPECT_CALL(ares_, ProcessFd(kAresChannel, kAresFd, ARES_SOCKET_BAD))
.WillOnce(InvokeWithoutArgs(this, &DNSClientTest::CallReplyCB));
+ ExpectPostCompletionTask();
CallDNSRead();
- ASSERT_TRUE(dns_client_->address().IsValid());
- IPAddress ipaddr(dns_client_->address().family());
+
+ // Make sure that the address value is correct as held in the DNSClient.
+ ASSERT_TRUE(dns_client_->address_.IsValid());
+ IPAddress ipaddr(dns_client_->address_.family());
ASSERT_TRUE(ipaddr.SetAddressFromString(kResult));
- EXPECT_TRUE(ipaddr.Equals(dns_client_->address()));
+ EXPECT_TRUE(ipaddr.Equals(dns_client_->address_));
+
+ // Make sure the callback gets called with a success result, and save
+ // the callback address argument in |address_result_|.
+ EXPECT_CALL(callback_target_, CallTarget(IsSuccess(true), _))
+ .WillOnce(Invoke(this, &DNSClientTest::SaveCallbackArgs));
+ CallCompletion();
+
+ // Make sure the address was successfully passed to the callback.
+ EXPECT_TRUE(ipaddr.Equals(address_result_));
+ EXPECT_TRUE(dns_client_->address_.IsDefault());
+ }
+
+ void SaveCallbackArgs(const Error &error, const IPAddress &address) {
+ error_result_.CopyFrom(error);
+ address_result_ = address;
+ }
+
+ void ExpectPostCompletionTask() {
+ EXPECT_CALL(dispatcher_, PostTask(_));
+ }
+
+ void ExpectReset() {
+ EXPECT_TRUE(dns_client_->address_.family() == IPAddress::kFamilyIPv4);
+ EXPECT_TRUE(dns_client_->address_.IsDefault());
+ EXPECT_TRUE(dns_client_->task_factory_.empty());
+ EXPECT_FALSE(dns_client_->resolver_state_.get());
}
protected:
@@ -168,15 +212,16 @@
DNSCallbackTarget()
: callback_(NewCallback(this, &DNSCallbackTarget::CallTarget)) {}
- MOCK_METHOD1(CallTarget, void(bool success));
- Callback1<bool>::Type *callback() { return callback_.get(); }
+ MOCK_METHOD2(CallTarget, void(const Error &error,
+ const IPAddress &address));
+ DNSClient::ClientCallback *callback() { return callback_.get(); }
private:
- scoped_ptr<Callback1<bool>::Type> callback_;
+ scoped_ptr<DNSClient::ClientCallback> callback_;
};
scoped_ptr<DNSClient> dns_client_;
- MockEventDispatcher dispatcher_;
+ StrictMock<MockEventDispatcher> dispatcher_;
string queued_request_;
StrictMock<DNSCallbackTarget> callback_target_;
StrictMock<MockAres> ares_;
@@ -185,6 +230,8 @@
struct timeval ares_timeout_;
struct hostent hostent_;
int ares_result_;
+ Error error_result_;
+ IPAddress address_result_;
};
class SentinelIOHandler : public IOHandler {
@@ -197,14 +244,15 @@
vector<string> dns_servers;
dns_servers.push_back(kGoodServer);
CreateClient(dns_servers, kAresTimeoutMS);
- EXPECT_TRUE(dns_client_->address().family() == IPAddress::kFamilyIPv4);
- EXPECT_TRUE(dns_client_->address().IsDefault());
+ ExpectReset();
}
// Receive error because no DNS servers were specified.
TEST_F(DNSClientTest, NoServers) {
CreateClient(vector<string>(), kAresTimeoutMS);
- EXPECT_FALSE(dns_client_->Start(kGoodName));
+ Error error;
+ EXPECT_FALSE(dns_client_->Start(kGoodName, &error));
+ EXPECT_EQ(Error::kInvalidArguments, error.type());
}
// Receive error because the DNS server IP address is invalid.
@@ -212,7 +260,9 @@
vector<string> dns_servers;
dns_servers.push_back(kBadServer);
CreateClient(dns_servers, kAresTimeoutMS);
- ASSERT_FALSE(dns_client_->Start(kGoodName));
+ Error error;
+ ASSERT_FALSE(dns_client_->Start(kGoodName, &error));
+ EXPECT_EQ(Error::kInvalidArguments, error.type());
}
// Setup error because InitOptions failed.
@@ -222,13 +272,17 @@
CreateClient(dns_servers, kAresTimeoutMS);
EXPECT_CALL(ares_, InitOptions(_, _, _))
.WillOnce(Return(ARES_EBADFLAGS));
- EXPECT_FALSE(dns_client_->Start(kGoodName));
+ Error error;
+ EXPECT_FALSE(dns_client_->Start(kGoodName, &error));
+ EXPECT_EQ(Error::kOperationFailed, error.type());
}
// Fail a second request because one is already in progress.
TEST_F(DNSClientTest, MultipleRequest) {
StartValidRequest();
- ASSERT_FALSE(dns_client_->Start(kGoodName));
+ Error error;
+ ASSERT_FALSE(dns_client_->Start(kGoodName, &error));
+ EXPECT_EQ(Error::kInProgress, error.type());
}
TEST_F(DNSClientTest, GoodRequest) {
@@ -277,10 +331,18 @@
EXPECT_CALL(time_, GetTimeMonotonic(_))
.WillOnce(DoAll(SetArgumentPointee<0>(init_time_val), Return(0)))
.WillRepeatedly(DoAll(SetArgumentPointee<0>(time_val_), Return(0)));
- EXPECT_CALL(callback_target_, CallTarget(false));
+ EXPECT_CALL(callback_target_, CallTarget(IsSuccess(false), _))
+ .Times(0);
EXPECT_CALL(ares_, Destroy(kAresChannel));
- ASSERT_FALSE(dns_client_->Start(kGoodName));
- EXPECT_EQ(string(DNSClient::kErrorTimedOut), dns_client_->error());
+ Error error;
+ // Expect the DNSClient to post a completion task. However this task will
+ // never run since the Stop() gets called before returning. We confirm
+ // that the task indeed gets canceled below in ExpectReset().
+ ExpectPostCompletionTask();
+ ASSERT_FALSE(dns_client_->Start(kGoodName, &error));
+ EXPECT_EQ(Error::kOperationTimeout, error.type());
+ EXPECT_EQ(string(DNSClient::kErrorTimedOut), error.message());
+ ExpectReset();
}
// Failed request due to timeout within the dns_client.
@@ -289,8 +351,11 @@
EXPECT_CALL(ares_, ProcessFd(kAresChannel,
ARES_SOCKET_BAD, ARES_SOCKET_BAD));
AdvanceTime(kAresTimeoutMS);
- EXPECT_CALL(callback_target_, CallTarget(false));
+ ExpectPostCompletionTask();
CallTimeout();
+ EXPECT_CALL(callback_target_, CallTarget(
+ IsFailure(Error::kOperationTimeout, DNSClient::kErrorTimedOut), _));
+ CallCompletion();
}
// Failed request due to timeout reported by ARES.
@@ -300,9 +365,11 @@
ares_result_ = ARES_ETIMEOUT;
EXPECT_CALL(ares_, ProcessFd(kAresChannel, ARES_SOCKET_BAD, ARES_SOCKET_BAD))
.WillOnce(InvokeWithoutArgs(this, &DNSClientTest::CallReplyCB));
- EXPECT_CALL(callback_target_, CallTarget(false));
+ ExpectPostCompletionTask();
CallTimeout();
- EXPECT_EQ(string(DNSClient::kErrorTimedOut), dns_client_->error());
+ EXPECT_CALL(callback_target_, CallTarget(
+ IsFailure(Error::kOperationTimeout, DNSClient::kErrorTimedOut), _));
+ CallCompletion();
}
// Failed request due to "host not found" reported by ARES.
@@ -312,9 +379,11 @@
ares_result_ = ARES_ENOTFOUND;
EXPECT_CALL(ares_, ProcessFd(kAresChannel, kAresFd, ARES_SOCKET_BAD))
.WillOnce(InvokeWithoutArgs(this, &DNSClientTest::CallReplyCB));
- EXPECT_CALL(callback_target_, CallTarget(false));
+ ExpectPostCompletionTask();
CallDNSRead();
- EXPECT_EQ(string(DNSClient::kErrorNotFound), dns_client_->error());
+ EXPECT_CALL(callback_target_, CallTarget(
+ IsFailure(Error::kOperationFailed, DNSClient::kErrorNotFound), _));
+ CallCompletion();
}
// Make sure IOHandles are deallocated when GetSock() reports them gone.
@@ -327,7 +396,8 @@
.WillOnce(Return(io_handler));
EXPECT_CALL(dispatcher_, PostDelayedTask(_, kAresWaitMS));
SetActive();
- ASSERT_TRUE(dns_client_->Start(kGoodName));
+ Error error;
+ ASSERT_TRUE(dns_client_->Start(kGoodName, &error));
AdvanceTime(kAresWaitMS);
SetInActive();
EXPECT_CALL(*io_handler, Die());
@@ -347,7 +417,8 @@
.WillOnce(Return(io_handler));
EXPECT_CALL(dispatcher_, PostDelayedTask(_, kAresWaitMS));
SetActive();
- ASSERT_TRUE(dns_client_->Start(kGoodName));
+ Error error;
+ ASSERT_TRUE(dns_client_->Start(kGoodName, &error));
EXPECT_CALL(*io_handler, Die());
EXPECT_CALL(ares_, Destroy(kAresChannel));
dns_client_->Stop();
diff --git a/http_proxy.cc b/http_proxy.cc
index 2075c65..9e5e83e 100644
--- a/http_proxy.cc
+++ b/http_proxy.cc
@@ -190,13 +190,13 @@
}
// DNSClient callback that fires when the DNS request completes.
-void HTTPProxy::GetDNSResult(bool result) {
- if (!result) {
+void HTTPProxy::GetDNSResult(const Error &error, const IPAddress &address) {
+ if (!error.IsSuccess()) {
SendClientError(502, string("Could not resolve hostname: ") +
- dns_client_->error());
+ error.message());
return;
}
- ConnectServer(dns_client_->address(), server_port_);
+ ConnectServer(address, server_port_);
}
// IOReadyHandler callback routine which fires when the asynchronous Connect()
@@ -301,8 +301,9 @@
}
} else {
VLOG(3) << "Looking up host: " << server_hostname_;
- if (!dns_client_->Start(server_hostname_)) {
- SendClientError(502, "Could not resolve hostname");
+ Error error;
+ if (!dns_client_->Start(server_hostname_, &error)) {
+ SendClientError(502, "Could not resolve hostname: " + error.message());
return false;
}
state_ = kStateLookupServer;
diff --git a/http_proxy.h b/http_proxy.h
index 1541917..b2d4b83 100644
--- a/http_proxy.h
+++ b/http_proxy.h
@@ -19,8 +19,9 @@
namespace shill {
class AsyncConnection;
-class EventDispatcher;
class DNSClient;
+class Error;
+class EventDispatcher;
class InputData;
class IOHandler;
class IPAddress;
@@ -93,7 +94,7 @@
void AcceptClient(int fd);
bool ConnectServer(const IPAddress &address, int port);
- void GetDNSResult(bool result);
+ void GetDNSResult(const Error &error, const IPAddress &address);
void OnConnectCompletion(bool success, int fd);
bool ParseClientRequest();
bool ProcessLastHeaderLine();
@@ -119,7 +120,8 @@
ConnectionRefPtr connection_;
scoped_ptr<Callback1<int>::Type> accept_callback_;
scoped_ptr<Callback2<bool, int>::Type> connect_completion_callback_;
- scoped_ptr<Callback1<bool>::Type> dns_client_callback_;
+ scoped_ptr<Callback2<const Error &, const IPAddress &>::Type>
+ dns_client_callback_;
scoped_ptr<Callback1<InputData *>::Type> read_client_callback_;
scoped_ptr<Callback1<InputData *>::Type> read_server_callback_;
scoped_ptr<Callback1<int>::Type> write_client_callback_;
diff --git a/http_proxy_unittest.cc b/http_proxy_unittest.cc
index 6fba3f7..5318b0b 100644
--- a/http_proxy_unittest.cc
+++ b/http_proxy_unittest.cc
@@ -273,13 +273,9 @@
EXPECT_EQ(line, proxy_.client_headers_[0] + "\r\n");
}
void ExpectDNSRequest(const string &host, bool return_value) {
- EXPECT_CALL(*dns_client_, Start(StrEq(host)))
+ EXPECT_CALL(*dns_client_, Start(StrEq(host), _))
.WillOnce(Return(return_value));
}
- void ExpectDNSFailure(const string &error) {
- EXPECT_CALL(*dns_client_, error())
- .WillOnce(ReturnRef(error));
- }
void ExpectAsyncConnect(const string &address, int port,
bool return_value) {
EXPECT_CALL(*server_async_connection_, Start(IsIPAddress(address), port))
@@ -354,8 +350,14 @@
void AcceptClient(int fd) {
proxy_.AcceptClient(fd);
}
- void GetDNSResult(bool result) {
- proxy_.GetDNSResult(result);
+ void GetDNSResultFailure(const string &error_msg) {
+ Error error(Error::kOperationFailed, error_msg);
+ IPAddress address(IPAddress::kFamilyUnknown);
+ proxy_.GetDNSResult(error, address);
+ }
+ void GetDNSResultSuccess(const IPAddress &address) {
+ Error error;
+ proxy_.GetDNSResult(error, address);
}
void OnConnectCompletion(bool result, int sockfd) {
proxy_.OnConnectCompletion(result, sockfd);
@@ -411,9 +413,7 @@
ReadFromClient(CreateRequest(url, http_version, extra_lines));
IPAddress addr(IPAddress::kFamilyIPv4);
EXPECT_TRUE(addr.SetAddressFromString(kServerAddress));
- EXPECT_CALL(*dns_client_, address())
- .WillOnce(ReturnRef(addr));;
- GetDNSResult(true);
+ GetDNSResultSuccess(addr);
}
void SetupConnect() {
SetupConnectWithRequest("/", "1.1", "Host: www.chromium.org:40506");
@@ -617,8 +617,7 @@
ReadFromClient(CreateRequest("/", "1.1", "Host: www.chromium.org:40506"));
ExpectClientResult();
const std::string not_found_error(DNSClient::kErrorNotFound);
- ExpectDNSFailure(not_found_error);
- GetDNSResult(false);
+ GetDNSResultFailure(not_found_error);
ExpectClientError(502, string("Could not resolve hostname: ") +
not_found_error);
}
diff --git a/http_request.cc b/http_request.cc
index 055b62e..0810c45 100644
--- a/http_request.cc
+++ b/http_request.cc
@@ -13,6 +13,7 @@
#include "shill/async_connection.h"
#include "shill/connection.h"
#include "shill/dns_client.h"
+#include "shill/error.h"
#include "shill/event_dispatcher.h"
#include "shill/http_url.h"
#include "shill/ip_address.h"
@@ -46,7 +47,6 @@
result_callback_(NULL),
read_event_callback_(NULL),
task_factory_(this),
- idle_timeout_(NULL),
dns_client_(
new DNSClient(IPAddress::kFamilyIPv4,
connection->interface_name(),
@@ -69,8 +69,8 @@
HTTPRequest::Result HTTPRequest::Start(
const HTTPURL &url,
- Callback1<int>::Type *read_event_callback,
- Callback1<Result>::Type *result_callback) {
+ Callback1<const ByteString &>::Type *read_event_callback,
+ Callback2<Result, const ByteString &>::Type *result_callback) {
VLOG(3) << "In " << __func__;
DCHECK(!is_running_);
@@ -94,8 +94,9 @@
}
} else {
VLOG(3) << "Looking up host: " << server_hostname_;
- if (!dns_client_->Start(server_hostname_)) {
- LOG(ERROR) << "Failed to start DNS client";
+ Error error;
+ if (!dns_client_->Start(server_hostname_, &error)) {
+ LOG(ERROR) << "Failed to start DNS client: " << error.message();
Stop();
return kResultDNSFailure;
}
@@ -122,7 +123,6 @@
connection_->ReleaseRouting();
dns_client_->Stop();
- idle_timeout_ = NULL;
is_running_ = false;
result_callback_ = NULL;
read_event_callback_ = NULL;
@@ -155,22 +155,21 @@
}
// DNSClient callback that fires when the DNS request completes.
-void HTTPRequest::GetDNSResult(bool result) {
+void HTTPRequest::GetDNSResult(const Error &error, const IPAddress &address) {
VLOG(3) << "In " << __func__;
- if (!result) {
- const string &error = dns_client_->error();
+ if (!error.IsSuccess()) {
LOG(ERROR) << "Could not resolve hostname "
<< server_hostname_
<< ": "
- << error;
- if (error == DNSClient::kErrorTimedOut) {
+ << error.message();
+ if (error.message() == DNSClient::kErrorTimedOut) {
SendStatus(kResultDNSTimeout);
} else {
SendStatus(kResultDNSFailure);
}
return;
}
- ConnectServer(dns_client_->address(), server_port_);
+ ConnectServer(address, server_port_);
}
// AsyncConnection callback routine which fires when the asynchronous Connect()
@@ -203,27 +202,33 @@
}
response_data_.Append(ByteString(data->buf, data->len));
- if (read_event_callback_) {
- read_event_callback_->Run(data->len);
- }
StartIdleTimeout(kInputTimeoutSeconds, kResultResponseTimeout);
+ if (read_event_callback_) {
+ read_event_callback_->Run(response_data_);
+ }
}
void HTTPRequest::SendStatus(Result result) {
- if (result_callback_) {
- result_callback_->Run(result);
- }
+ // Save copies on the stack, since Stop() will remove them.
+ Callback2<Result, const ByteString &>::Type *result_callback =
+ result_callback_;
+ const ByteString response_data(response_data_);
Stop();
+
+ // Call the callback last, since it may delete us and |this| may no longer
+ // be valid.
+ if (result_callback) {
+ result_callback->Run(result, response_data);
+ }
}
// Start a timeout for "the next event".
void HTTPRequest::StartIdleTimeout(int timeout_seconds, Result timeout_result) {
- if (idle_timeout_) {
- idle_timeout_->Cancel();
- }
timeout_result_ = timeout_result;
- idle_timeout_ = task_factory_.NewRunnableMethod(&HTTPRequest::TimeoutTask);
- dispatcher_->PostDelayedTask(idle_timeout_, timeout_seconds * 1000);
+ task_factory_.RevokeAll();
+ dispatcher_->PostDelayedTask(
+ task_factory_.NewRunnableMethod(&HTTPRequest::TimeoutTask),
+ timeout_seconds * 1000);
}
void HTTPRequest::TimeoutTask() {
diff --git a/http_request.h b/http_request.h
index 1700ab7..75545e5 100644
--- a/http_request.h
+++ b/http_request.h
@@ -21,6 +21,7 @@
class AsyncConnection;
class DNSClient;
+class Error;
class EventDispatcher;
class HTTPURL;
class InputData;
@@ -53,27 +54,32 @@
virtual ~HTTPRequest();
// Start an http GET request to the URL |url|. Whenever any data is
- // read from the server, |read_event_callback| is called with the number
- // of bytes read. This callback could be called more than once as data
- // arrives. All callbacks can access the data as it is read in by
- // using the response_data() getter below.
+ // read from the server, |read_event_callback| is called with the
+ // current contents of the response data coming from the server.
+ // This callback could be called more than once as data arrives.
//
// When the transaction completes, |result_callback| will be called with
- // the final status from the transaction. |result_callback| will not be
+ // the final status from the transaction. It is valid for the callback
+ // function to destroy this HTTPRequest object, because at this time all
+ // object state has already been cleaned up. |result_callback| will not be
// called if either the Start() call fails or if Stop() is called before
// the transaction completes.
//
// This (Start) function returns a failure result if the request
// failed during initialization, or kResultInProgress if the request
// has started successfully and is now in progress.
- virtual Result Start(const HTTPURL &url,
- Callback1<int>::Type *read_event_callback,
- Callback1<Result>::Type *result_callback);
+ virtual Result Start(
+ const HTTPURL &url,
+ Callback1<const ByteString &>::Type *read_event_callback,
+ Callback2<Result, const ByteString &>::Type *result_callback);
// Stop the current HTTPRequest. No callback is called as a side
// effect of this function.
virtual void Stop();
+ // Returns the data received so far from the server in the current
+ // request. This data is available only while the request is active,
+ // and before the result callback is called.
virtual const ByteString &response_data() const { return response_data_; }
private:
@@ -89,7 +95,7 @@
static const char kHTTPRequestTemplate[];
bool ConnectServer(const IPAddress &address, int port);
- void GetDNSResult(bool result);
+ void GetDNSResult(const Error &error, const IPAddress &address);
void OnConnectCompletion(bool success, int fd);
void ReadFromServer(InputData *data);
void SendStatus(Result result);
@@ -102,13 +108,13 @@
Sockets *sockets_;
scoped_ptr<Callback2<bool, int>::Type> connect_completion_callback_;
- scoped_ptr<Callback1<bool>::Type> dns_client_callback_;
+ scoped_ptr<Callback2<const Error &, const IPAddress &>::Type>
+ dns_client_callback_;
scoped_ptr<Callback1<InputData *>::Type> read_server_callback_;
scoped_ptr<Callback1<int>::Type> write_server_callback_;
- Callback1<Result>::Type *result_callback_;
- Callback1<int>::Type *read_event_callback_;
+ Callback2<Result, const ByteString &>::Type *result_callback_;
+ Callback1<const ByteString &>::Type *read_event_callback_;
ScopedRunnableMethodFactory<HTTPRequest> task_factory_;
- CancelableTask *idle_timeout_;
scoped_ptr<IOHandler> read_server_handler_;
scoped_ptr<IOHandler> write_server_handler_;
scoped_ptr<DNSClient> dns_client_;
diff --git a/http_request_unittest.cc b/http_request_unittest.cc
index d29fcd6..d24d13d 100644
--- a/http_request_unittest.cc
+++ b/http_request_unittest.cc
@@ -62,6 +62,10 @@
return ip_address.Equals(arg);
}
+MATCHER_P(ByteStringMatches, byte_string, "") {
+ return byte_string.Equals(arg);
+}
+
class HTTPRequestTest : public Test {
public:
HTTPRequestTest()
@@ -84,18 +88,21 @@
result_callback_(
NewCallback(this, &CallbackTarget::ResultCallTarget)) {}
- MOCK_METHOD1(ReadEventCallTarget, void(int byte_count));
- MOCK_METHOD1(ResultCallTarget, void(HTTPRequest::Result));
- Callback1<int>::Type *read_event_callback() {
+ MOCK_METHOD1(ReadEventCallTarget, void(const ByteString &response_data));
+ MOCK_METHOD2(ResultCallTarget, void(HTTPRequest::Result result,
+ const ByteString &response_data));
+ Callback1<const ByteString &>::Type *read_event_callback() {
return read_event_callback_.get();
}
- Callback1<HTTPRequest::Result>::Type *result_callback() {
+ Callback2<HTTPRequest::Result,
+ const ByteString &>::Type *result_callback() {
return result_callback_.get();
}
private:
- scoped_ptr<Callback1<int>::Type> read_event_callback_;
- scoped_ptr<Callback1<HTTPRequest::Result>::Type> result_callback_;
+ scoped_ptr<Callback1<const ByteString &>::Type> read_event_callback_;
+ scoped_ptr<Callback2<HTTPRequest::Result, const ByteString &>::Type>
+ result_callback_;
};
virtual void SetUp() {
@@ -140,7 +147,6 @@
EXPECT_FALSE(request_->result_callback_);
EXPECT_FALSE(request_->read_event_callback_);
EXPECT_TRUE(request_->task_factory_.empty());
- EXPECT_FALSE(request_->idle_timeout_);
EXPECT_FALSE(request_->read_server_handler_.get());
EXPECT_FALSE(request_->write_server_handler_.get());
EXPECT_EQ(dns_client_, request_->dns_client_.get());
@@ -182,13 +188,9 @@
EXPECT_NE(string::npos, response_string.find(expected_response_data));
}
void ExpectDNSRequest(const string &host, bool return_value) {
- EXPECT_CALL(*dns_client_, Start(StrEq(host)))
+ EXPECT_CALL(*dns_client_, Start(StrEq(host), _))
.WillOnce(Return(return_value));
}
- void ExpectDNSFailure(const string &error) {
- EXPECT_CALL(*dns_client_, error())
- .WillOnce(ReturnRef(error));
- }
void ExpectAsyncConnect(const string &address, int port,
bool return_value) {
EXPECT_CALL(*server_async_connection_, Start(IsIPAddress(address), port))
@@ -234,22 +236,30 @@
EXPECT_CALL(*connection_.get(), ReleaseRouting());
}
void ExpectResultCallback(HTTPRequest::Result result) {
- EXPECT_CALL(target_, ResultCallTarget(result));
+ EXPECT_CALL(target_, ResultCallTarget(result, _));
}
- void InvokeResultVerify(HTTPRequest::Result result) {
+ void InvokeResultVerify(HTTPRequest::Result result,
+ const ByteString &response_data) {
EXPECT_EQ(HTTPRequest::kResultSuccess, result);
- EXPECT_TRUE(expected_response_.Equals(request_->response_data()));
+ EXPECT_TRUE(expected_response_.Equals(response_data));
}
void ExpectResultCallbackWithResponse(const string &response) {
expected_response_ = ByteString(response, false);
- EXPECT_CALL(target_, ResultCallTarget(HTTPRequest::kResultSuccess))
+ EXPECT_CALL(target_, ResultCallTarget(HTTPRequest::kResultSuccess, _))
.WillOnce(Invoke(this, &HTTPRequestTest::InvokeResultVerify));
}
- void ExpectReadEventCallback(int byte_count) {
- EXPECT_CALL(target_, ReadEventCallTarget(byte_count));
+ void ExpectReadEventCallback(const string &response) {
+ ByteString response_data(response, false);
+ EXPECT_CALL(target_, ReadEventCallTarget(ByteStringMatches(response_data)));
}
- void GetDNSResult(bool result) {
- request_->GetDNSResult(result);
+ void GetDNSResultFailure(const string &error_msg) {
+ Error error(Error::kOperationFailed, error_msg);
+ IPAddress address(IPAddress::kFamilyUnknown);
+ request_->GetDNSResult(error, address);
+ }
+ void GetDNSResultSuccess(const IPAddress &address) {
+ Error error;
+ request_->GetDNSResult(error, address);
}
void OnConnectCompletion(bool result, int sockfd) {
request_->OnConnectCompletion(result, sockfd);
@@ -277,9 +287,7 @@
EXPECT_EQ(HTTPRequest::kResultInProgress, StartRequest(url));
IPAddress addr(IPAddress::kFamilyIPv4);
EXPECT_TRUE(addr.SetAddressFromString(kServerAddress));
- EXPECT_CALL(*dns_client_, address())
- .WillOnce(ReturnRef(addr));;
- GetDNSResult(true);
+ GetDNSResultSuccess(addr);
}
void SetupConnect() {
SetupConnectWithURL(kTextURL, kTextSiteName);
@@ -366,11 +374,9 @@
ExpectRouteRequest();
ExpectDNSRequest(kTextSiteName, true);
EXPECT_EQ(HTTPRequest::kResultInProgress, StartRequest(kTextURL));
- const string error(DNSClient::kErrorNoData);
- ExpectDNSFailure(error);
ExpectResultCallback(HTTPRequest::kResultDNSFailure);
ExpectStop();
- GetDNSResult(false);
+ GetDNSResultFailure(DNSClient::kErrorNoData);
ExpectReset();
}
@@ -378,11 +384,10 @@
ExpectRouteRequest();
ExpectDNSRequest(kTextSiteName, true);
EXPECT_EQ(HTTPRequest::kResultInProgress, StartRequest(kTextURL));
- const string error(DNSClient::kErrorTimedOut);
- ExpectDNSFailure(error);
ExpectResultCallback(HTTPRequest::kResultDNSTimeout);
ExpectStop();
- GetDNSResult(false);
+ const string error(DNSClient::kErrorTimedOut);
+ GetDNSResultFailure(error);
ExpectReset();
}
@@ -436,13 +441,13 @@
TEST_F(HTTPRequestTest, ResponseData) {
SetupConnectComplete();
const string response0("hello");
- ExpectReadEventCallback(response0.length());
+ ExpectReadEventCallback(response0);
ExpectSetInputTimeout();
ReadFromServer(response0);
ExpectInResponse(response0);
const string response1(" to you");
- ExpectReadEventCallback(response1.length());
+ ExpectReadEventCallback(response0 + response1);
ExpectSetInputTimeout();
ReadFromServer(response1);
ExpectInResponse(response1);
@@ -450,6 +455,7 @@
ExpectResultCallbackWithResponse(response0 + response1);
ExpectStop();
ReadFromServer("");
+ ExpectReset();
}
} // namespace shill
diff --git a/mock_dns_client.h b/mock_dns_client.h
index 73cf62d..e8fcc91 100644
--- a/mock_dns_client.h
+++ b/mock_dns_client.h
@@ -17,10 +17,8 @@
MockDNSClient();
virtual ~MockDNSClient();
- MOCK_METHOD1(Start, bool(const std::string &hostname));
+ MOCK_METHOD2(Start, bool(const std::string &hostname, Error *error));
MOCK_METHOD0(Stop, void());
- MOCK_CONST_METHOD0(address, const IPAddress &());
- MOCK_CONST_METHOD0(error, const std::string &());
private:
DISALLOW_COPY_AND_ASSIGN(MockDNSClient);
diff --git a/mock_http_request.h b/mock_http_request.h
index dd8c911..b759cc6 100644
--- a/mock_http_request.h
+++ b/mock_http_request.h
@@ -20,8 +20,8 @@
MOCK_METHOD3(Start, HTTPRequest::Result(
const HTTPURL &url,
- Callback1<int>::Type *read_event_callback,
- Callback1<Result>::Type *result_callback));
+ Callback1<const ByteString &>::Type *read_event_callback,
+ Callback2<Result, const ByteString &>::Type *result_callback));
MOCK_METHOD0(Stop, void());
MOCK_CONST_METHOD0(response_data, const ByteString &());
diff --git a/portal_detector.cc b/portal_detector.cc
index 35228d1..7bbb965 100644
--- a/portal_detector.cc
+++ b/portal_detector.cc
@@ -162,9 +162,8 @@
}
}
-void PortalDetector::RequestReadCallback(int /*read_length*/) {
+void PortalDetector::RequestReadCallback(const ByteString &response_data) {
const string response_expected(kResponseExpected);
- const ByteString &response_data = request_->response_data();
bool expected_length_received = false;
int compare_length = 0;
if (response_data.GetLength() < response_expected.length()) {
@@ -188,7 +187,8 @@
}
}
-void PortalDetector::RequestResultCallback(HTTPRequest::Result result) {
+void PortalDetector::RequestResultCallback(
+ HTTPRequest::Result result, const ByteString &/*response_data*/) {
CompleteAttempt(GetPortalResultForRequestResult(result));
}
diff --git a/portal_detector.h b/portal_detector.h
index 1c1079e..2b4148c 100644
--- a/portal_detector.h
+++ b/portal_detector.h
@@ -22,6 +22,7 @@
namespace shill {
+class ByteString;
class EventDispatcher;
class PortalDetector;
class Time;
@@ -120,8 +121,9 @@
static const char kStatusTimeoutString[];
void CompleteAttempt(Result result);
- void RequestReadCallback(int read_length);
- void RequestResultCallback(HTTPRequest::Result result);
+ void RequestReadCallback(const ByteString &response_data);
+ void RequestResultCallback(HTTPRequest::Result result,
+ const ByteString &response_data);
void StartAttempt();
void StartAttemptTask();
void StopAttempt();
@@ -133,8 +135,9 @@
EventDispatcher *dispatcher_;
Callback1<const Result &>::Type *portal_result_callback_;
scoped_ptr<HTTPRequest> request_;
- scoped_ptr<Callback1<int>::Type> request_read_callback_;
- scoped_ptr<Callback1<HTTPRequest::Result>::Type> request_result_callback_;
+ scoped_ptr<Callback1<const ByteString &>::Type> request_read_callback_;
+ scoped_ptr<Callback2<HTTPRequest::Result, const ByteString &>::Type>
+ request_result_callback_;
Sockets sockets_;
ScopedRunnableMethodFactory<PortalDetector> task_factory_;
Time *time_;
diff --git a/portal_detector_unittest.cc b/portal_detector_unittest.cc
index 6beb68c..7119cc3 100644
--- a/portal_detector_unittest.cc
+++ b/portal_detector_unittest.cc
@@ -166,9 +166,7 @@
void AppendReadData(const string &read_data) {
response_data_.Append(ByteString(read_data, false));
- EXPECT_CALL(*http_request_, response_data())
- .WillOnce(ReturnRef(response_data_));
- portal_detector_->RequestReadCallback(response_data_.GetLength());
+ portal_detector_->RequestReadCallback(response_data_);
}
private:
@@ -285,7 +283,8 @@
for (int i = 0; i < PortalDetector::kMaxRequestAttempts; i++) {
portal_detector()->StartAttemptTask();
AdvanceTime(PortalDetector::kMinTimeBetweenAttemptsSeconds * 1000);
- portal_detector()->RequestResultCallback(HTTPRequest::kResultDNSFailure);
+ portal_detector()->RequestResultCallback(HTTPRequest::kResultDNSFailure,
+ response_data());
}
ExpectReset();