Skip to content

Commit

Permalink
Fix bug : #9
Browse files Browse the repository at this point in the history 
When TCPClient is connecting to a host and it is doing a DNS resolving,
the user calls TCPClient::Disconnect, that will cause a crush at
Connector::Cancel::chan_->DisableAllEvent()

void Connector::Cancel() {
    LOG_INFO << "Cancel to connect " << remote_addr_ << " status=" << StatusToString();
    assert(loop_->IsInLoopThread());
    if (dns_resolver_) {
        dns_resolver_->Cancel();
    }

    assert(timer_);
    timer_->Cancel();
    chan_->DisableAllEvent();
    chan_->Close();
}
  • Loading branch information
@zieckey
zieckey committed Mar 17, 2017
1 parent 4be5ade commit 9ab845a
Show file tree
Hide file tree
Showing 7 changed files with 70 additions and 44 deletions.
5 changes: 5 additions & 0 deletions .gitignore
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ vsprojects/.vs/
vsprojects/libevpp.VC.db
vsprojects/evnsq-test.vcxproj.filters
vsprojects/enc_temp_folder/
vsprojects/evnsq-test.vcxproj.user
vsprojects/libevpp-test.vcxproj.user

test/evpp-test
examples/echo/*/simple_echo
Expand All @@ -47,6 +49,7 @@ build-release

apps/evnsq/evnsqtail/evnsqtail
apps/evnsq/test/evnsqtest
apps/evnsq.bak/

3rdparty/libevent-2.1.5-beta/.vs/
3rdparty/libevent-2.1.5-beta/Win32/
Expand Down Expand Up @@ -93,3 +96,5 @@ benchmark/ioevent/libevent/dat.t0
benchmark/ioevent/libevent/pingpong_bench
benchmark/ioevent/libevent/runbench.tmp
tools/a.sh


4 changes: 1 addition & 3 deletions docs/quick_start_win32_vs2015.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ Go to `evpp/3rdparty/libevent-release-2.1.8-stable`
$ cmake -G "Visual Studio 14" ..
$ start libevent.sln
... # here you can use Visual Studio 2015 to compile the three libevent project event,event_core,event_extra in debug and release mode.
$ cd
$
$ cp lib/Debug/*.* ../../../vsprojects/bin/Debug/
$ cp lib/Release/*.* ../../../vsprojects/bin/Release/
$ cp -rf ../include/event2 ../../wininclude/
Expand All @@ -36,8 +36,6 @@ Note 1: We have modified the source code of libevent-release-2.1.8-stable as bel
1. libevent-release-2.1.8-stable/CMakeList.txt : Add 'set(EVENT__DISABLE_OPENSSL ON)' to disable OPENSSL support
2. libevent-release-2.1.8-stable/cmake/VersionViaGit.cmake : Delete or comment the tow lines: 'find_package(Git)' and 'include(FindGit)'

Note 2: The precompied windows static library have been uploaded to the proper dir : `vsprojects/bin/Debug` . So you can skip this step.

### Compile evpp

$ cd ../../../
Expand Down
22 changes: 17 additions & 5 deletions evpp/connector.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,8 @@ void Connector::Start() {
auto index = remote_addr_.rfind(':');
assert(index != std::string::npos);
auto host = std::string(remote_addr_.data(), index);
dns_resolver_.reset(new DNSResolver(loop_, host, timeout_, std::bind(&Connector::OnDNSResolved, shared_from_this(), std::placeholders::_1)));
auto f = std::bind(&Connector::OnDNSResolved, shared_from_this(), std::placeholders::_1);
dns_resolver_ = std::make_shared<DNSResolver>(loop_, host, timeout_, f);
dns_resolver_->Start();
return;
}
Expand All @@ -70,8 +71,17 @@ void Connector::Cancel() {

assert(timer_);
timer_->Cancel();
chan_->DisableAllEvent();
chan_->Close();

if (status_ == kDNSResolving) {
assert(chan_.get() == NULL);
conn_fn_(-1, "");
}

if (chan_.get()) {
assert(status_ != kDNSResolving);
chan_->DisableAllEvent();
chan_->Close();
}
}

void Connector::Connect() {
Expand All @@ -97,7 +107,9 @@ void Connector::Connect() {

void Connector::HandleWrite() {
if (status_ == kDisconnected) {
// 这里有可能是超时了，但回调时间已经派发到队列中，后来才调用。
// The connecting may be timeout, but the write event handler has been
// dispatched in the EventLoop pending task queue, and next loop time the handle is invoked.
// So we need to check the status whether it is at a kDisconnected
LOG_INFO << "fd=" << chan_->fd() << " remote_addr=" << remote_addr_ << " receive write event when socket is closed";
return;
}
Expand All @@ -123,7 +135,7 @@ void Connector::HandleWrite() {
timer_->Cancel();
chan_->DisableAllEvent();
chan_->Close();
own_fd_ = false; // 将fd的所有权转移给TCPConn
own_fd_ = false; // Move the ownership of the fd to TCPConn
fd_ = INVALID_SOCKET;
status_ = kConnected;
}
Expand Down
2 changes: 1 addition & 1 deletion evpp/connector.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ class EVPP_EXPORT Connector : public std::enable_shared_from_this<Connector> {

std::unique_ptr<FdChannel> chan_;
std::unique_ptr<TimerEventWatcher> timer_;
std::unique_ptr<DNSResolver> dns_resolver_;
std::shared_ptr<DNSResolver> dns_resolver_;
NewConnectionCallback conn_fn_;
};
}
57 changes: 25 additions & 32 deletions evpp/dns_resolver.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,18 +12,10 @@ DNSResolver::~DNSResolver() {
LOG_INFO << "DNSResolver::~DNSResolver tid=" << std::this_thread::get_id() << " this=" << this;

assert(dnsbase_ == nullptr);

if (dns_req_) {
dns_req_ = nullptr;
}
}

void DNSResolver::Start() {
loop_->RunInLoop(std::bind(&DNSResolver::StartInLoop, this));
}

void DNSResolver::StartInLoop() {
LOG_INFO << "DNSResolver::StartInLoop tid=" << std::this_thread::get_id() << " this=" << this;
LOG_INFO << "DNSResolver::Start tid=" << std::this_thread::get_id() << " this=" << this;
assert(loop_->IsInLoopThread());

#if LIBEVENT_VERSION_NUMBER >= 0x02001500
Expand Down Expand Up @@ -65,31 +57,38 @@ void DNSResolver::SyncDNSResolve() {
}

void DNSResolver::Cancel() {
assert(loop_->IsInLoopThread());
if (timer_) {
loop_->RunInLoop(std::bind(&TimerEventWatcher::Cancel, timer_.get()));
timer_->Cancel();
}
}

void DNSResolver::AsyncWait() {
LOG_INFO << "DNSResolver::AsyncWait tid=" << std::this_thread::get_id() << " this=" << this;
timer_.reset(new TimerEventWatcher(loop_, std::bind(&DNSResolver::OnTimeout, this), timeout_));
timer_->SetCancelCallback(std::bind(&DNSResolver::OnCanceled, this));
timer_.reset(new TimerEventWatcher(loop_, std::bind(&DNSResolver::OnTimeout, shared_from_this()), timeout_));
timer_->SetCancelCallback(std::bind(&DNSResolver::OnCanceled, shared_from_this()));
timer_->Init();
timer_->AsyncWait();
}

void DNSResolver::OnTimeout() {
LOG_INFO << "DNSResolver::OnTimeout tid=" << std::this_thread::get_id() << " this=" << this;
#if LIBEVENT_VERSION_NUMBER >= 0x02001500
evdns_base_free(dnsbase_, 0);
evdns_getaddrinfo_cancel(dns_req_);
dnsbase_ = nullptr;
dns_req_ = nullptr;
#endif
functor_(this->addrs_);
}

void DNSResolver::OnCanceled() {
LOG_INFO << "DNSResolver::OnCanceled tid=" << std::this_thread::get_id() << " this=" << this;
#if LIBEVENT_VERSION_NUMBER >= 0x02001500
evdns_base_free(dnsbase_, 0);
evdns_getaddrinfo_cancel(dns_req_);
dnsbase_ = nullptr;
dns_req_ = nullptr;
#endif
}

Expand All @@ -106,11 +105,11 @@ void DNSResolver::AsyncDNSResolve() {

dnsbase_ = evdns_base_new(loop_->event_base(), 1);
dns_req_ = evdns_getaddrinfo(dnsbase_
, host_.c_str()
, nullptr /* no service name given */
, &hints
, &DNSResolver::OnResolved
, this);
, host_.c_str()
, nullptr /* no service name given */
, &hints
, &DNSResolver::OnResolved
, this);
assert(dnsbase_);
assert(dns_req_);
AsyncWait();
Expand All @@ -121,19 +120,16 @@ void DNSResolver::OnResolved(int errcode, struct addrinfo* addr) {
if (errcode != EVUTIL_EAI_CANCEL) {
timer_->Cancel();
LOG_ERROR << "dns resolve failed, "
<< ", error code: " << errcode
<< ", error msg: " << evutil_gai_strerror(errcode);
<< ", error code: " << errcode
<< ", error msg: " << evutil_gai_strerror(errcode);
} else {
LOG_WARN << "dns resolve cancel, may be timeout";
}

LOG_INFO << "delete dns ctx";
LOG_INFO << "delete dns base";
evdns_base_free(dnsbase_, 0);
dnsbase_ = nullptr;

//No route to host
//errno = EHOSTUNREACH;
//OnError();
functor_(this->addrs_);
return;
}
Expand All @@ -142,20 +138,17 @@ void DNSResolver::OnResolved(int errcode, struct addrinfo* addr) {
if (addr == nullptr) {
LOG_ERROR << "dns resolve error, addr can not be nullptr";

LOG_INFO << "delete dns ctx";
LOG_INFO << "delete dns base";
evdns_base_free(dnsbase_, 0);
dnsbase_ = nullptr;

//No route to host
//errno = EHOSTUNREACH;
//OnError();
functor_(this->addrs_);
return;
}


if (addr->ai_canonname) {
LOG_INFO << "resolve canon namne: " << addr->ai_canonname;
LOG_INFO << "resolve canon name: " << addr->ai_canonname;
}

for (struct addrinfo* rp = addr; rp != nullptr; rp = rp->ai_next) {
Expand All @@ -172,15 +165,15 @@ void DNSResolver::OnResolved(int errcode, struct addrinfo* addr) {
timer_->SetCancelCallback(TimerEventWatcher::Handler());
timer_->Cancel();

LOG_INFO << "delete dns ctx";
evdns_base_free(dnsbase_, 0);
LOG_INFO << "delete dns base";
evdns_base_free(dnsbase_, 0); //TODO Do we need to free dns_req_
dnsbase_ = nullptr;
functor_(this->addrs_);
}

void DNSResolver::OnResolved(int errcode, struct addrinfo* addr, void* arg) {
DNSResolver* t = (DNSResolver*)arg;
t->OnResolved(errcode, addr);
DNSResolver* dns = reinterpret_cast<DNSResolver*>(arg);
dns->OnResolved(errcode, addr);
}
#endif

Expand Down
5 changes: 2 additions & 3 deletions evpp/dns_resolver.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ struct evdns_getaddrinfo_request;
namespace evpp {
class EventLoop;
class TimerEventWatcher;
class EVPP_EXPORT DNSResolver {
class EVPP_EXPORT DNSResolver : public std::enable_shared_from_this<DNSResolver> {
public:
typedef std::function<void(const std::vector<struct in_addr>& addrs)> Functor;
DNSResolver(EventLoop* evloop, const std::string& host, Duration timeout, const Functor& f);
Expand All @@ -20,7 +20,6 @@ class EVPP_EXPORT DNSResolver {
return host_;
}
private:
void StartInLoop();
void SyncDNSResolve();
void AsyncDNSResolve();
void AsyncWait();
Expand All @@ -36,6 +35,6 @@ class EVPP_EXPORT DNSResolver {
Duration timeout_;
Functor functor_;
std::unique_ptr<TimerEventWatcher> timer_;
std::vector <struct in_addr> addrs_;
std::vector<struct in_addr> addrs_;
};
}
19 changes: 19 additions & 0 deletions test/tcp_client_reconnect_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
#include <evpp/tcp_conn.h>
#include <evpp/tcp_client.h>


#include <thread>

namespace {
Expand Down Expand Up @@ -102,4 +103,22 @@ TEST_UNIT(testTCPClientConnectFailed) {
H_TEST_ASSERT(evpp::GetActiveEventCount() == 0);
}

TEST_UNIT(testTCPClientDisconnectImmediately) {
std::shared_ptr<evpp::EventLoop> loop(new evpp::EventLoop);
std::shared_ptr<evpp::TCPClient> client(new evpp::TCPClient(loop.get(), "cmake.org:80", "TCPPingPongClient"));
client->SetConnectionCallback([&loop, &client](const evpp::TCPConnPtr& conn) {
H_TEST_ASSERT(!conn->IsConnected());
H_TEST_ASSERT(!loop->IsRunning());
auto f = [&loop]() { loop->Stop(); };
loop->RunAfter(300.0, f);
});
client->set_auto_reconnect(false);
client->Connect();
client->Disconnect();
loop->Run();
client.reset();
loop.reset();
H_TEST_ASSERT(evpp::GetActiveEventCount() == 0);
}


0 comments on commit 9ab845a

Please sign in to comment.