Skip to content

Commit

Permalink
Fix bugs:
Browse files Browse the repository at this point in the history
    1. httpc::Request does not retry when evhttp_make_request failed
    2. evnsq Client::HandleLoopkupdHTTPResponse coredump when response is null

Refactor:
    1. Remove httpc::Response::Response(Request* r)
  • Loading branch information
zieckey committed Jun 19, 2017
1 parent 7ea70dd commit 0c94eb2
Show file tree
Hide file tree
Showing 9 changed files with 226 additions and 76 deletions.
8 changes: 8 additions & 0 deletions apps/evnsq/client.cc
Expand Up @@ -114,6 +114,14 @@ void Client::HandleLoopkupdHTTPResponse(
const std::shared_ptr<evpp::httpc::Request>& request) {
DLOG_TRACE;

if (response.get() == nullptr) {
LOG_ERROR << "Request lookupd http://" << request->conn()->host() << ":"
<< request->conn()->port() << request->uri()
<< " failed, response is null";

return;
}

std::string body = response->body().ToString();
if (response->http_code() != 200) {
LOG_ERROR << "Request lookupd http://" << request->conn()->host() << ":"
Expand Down
43 changes: 29 additions & 14 deletions evpp/httpc/request.cc
Expand Up @@ -99,7 +99,7 @@ void Request::ExecuteInLoop() {
}
}

if (evhttp_make_request(conn_->evhttp_conn(), req, req_type, uri_.c_str())) {
if (evhttp_make_request(conn_->evhttp_conn(), req, req_type, uri_.c_str()) != 0) {
// At here conn_ has owned this req, so don't need to free it.
errmsg = "evhttp_make_request fail";
goto failed;
Expand All @@ -108,11 +108,26 @@ void Request::ExecuteInLoop() {
return;

failed:
LOG_ERROR << "http request failed: " << errmsg;
std::shared_ptr<Response> response;
// Retry
if (retried_ < retry_number_) {
LOG_WARN << "this=" << this << " http request failed : " << errmsg << " retried=" << retried_ << " max retry_time=" << retry_number_ << ". Try again.";
Retry();
return;
}

std::shared_ptr<Response> response(new Response(this, nullptr));
handler_(response);
}

void Request::Retry() {
retried_ += 1;
if (retry_interval_.IsZero()) {
ExecuteInLoop();
} else {
loop_->RunAfter(retry_interval_, std::bind(&Request::ExecuteInLoop, this));
}
}

void Request::HandleResponse(struct evhttp_request* r, void* v) {
Request* thiz = (Request*)v;
assert(thiz);
Expand Down Expand Up @@ -140,23 +155,23 @@ void Request::HandleResponse(struct evhttp_request* r) {
// Retry
if (retried_ < retry_number_) {
LOG_WARN << "this=" << this << " response_code=" << (r ? r->response_code : 0) << " retried=" << retried_ << " max retry_time=" << retry_number_ << ". Try again";
retried_ += 1;
ExecuteInLoop();
Retry();
return;
}


// Eventually this Request failed
std::shared_ptr<Response> response(new Response(this, r));

// Recycling the http Connection object
if (pool_) {
pool_->Put(conn_);
}
// Eventually this Request failed
std::shared_ptr<Response> response(new Response(this, r));

// Recycling the http Connection object
if (pool_) {
pool_->Put(conn_);
}

handler_(response);
handler_(response);
}



} // httpc
} // evpp

Expand Down
7 changes: 7 additions & 0 deletions evpp/httpc/request.h
Expand Up @@ -52,10 +52,14 @@ class EVPP_EXPORT Request {
void set_retry_number(int v) {
retry_number_ = v;
}
void set_retry_interval(Duration d) {
retry_interval_ = d;
}
private:
static void HandleResponse(struct evhttp_request* r, void* v);
void HandleResponse(struct evhttp_request* r);
void ExecuteInLoop();
void Retry();
protected:
static const std::string empty_;
private:
Expand All @@ -73,6 +77,9 @@ class EVPP_EXPORT Request {
// The max retry times. Set to 0 if you don't want to retry when failed.
// The total execution times is retry_number_+1
int retry_number_ = 2;

// Default : 1ms
Duration retry_interval_ = Duration(0.001);
};
typedef std::shared_ptr<Request> RequestPtr;

Expand Down
27 changes: 13 additions & 14 deletions evpp/httpc/response.cc
Expand Up @@ -8,23 +8,22 @@
namespace evpp {
namespace httpc {
Response::Response(Request* r, struct evhttp_request* evreq)
: request_(r), evreq_(evreq), http_code_(evreq->response_code) {
: request_(r), evreq_(evreq), http_code_(0) {
if (evreq) {
http_code_ = evreq->response_code;

#if LIBEVENT_VERSION_NUMBER >= 0x02001500
struct evbuffer* evbuf = evhttp_request_get_input_buffer(evreq);
size_t buffer_size = evbuffer_get_length(evbuf);
if (buffer_size > 0) {
this->body_ = evpp::Slice((char*)evbuffer_pullup(evbuf, -1), buffer_size);
}
struct evbuffer* evbuf = evhttp_request_get_input_buffer(evreq);
size_t buffer_size = evbuffer_get_length(evbuf);
if (buffer_size > 0) {
this->body_ = evpp::Slice((char*)evbuffer_pullup(evbuf, -1), buffer_size);
}
#else
if (evreq->input_buffer->off > 0) {
this->body_ = evpp::Slice((char*)evreq->input_buffer->buffer, evreq->input_buffer->off);
}
if (evreq->input_buffer->off > 0) {
this->body_ = evpp::Slice((char*)evreq->input_buffer->buffer, evreq->input_buffer->off);
}
#endif
}


Response::Response(Request* r)
: request_(r), evreq_(nullptr), http_code_(0) {
}
}

Response::~Response() {
Expand Down
1 change: 0 additions & 1 deletion evpp/httpc/response.h
Expand Up @@ -14,7 +14,6 @@ class EVPP_EXPORT Response {
public:
typedef std::map<evpp::Slice, evpp::Slice> Headers;
Response(Request* r, struct evhttp_request* evreq);
Response(Request* r);
~Response();

int http_code() const {
Expand Down
165 changes: 165 additions & 0 deletions test/http_client_test.cc
@@ -0,0 +1,165 @@
#include <iostream>

#include "test_common.h"

#include <stdio.h>
#include <stdlib.h>

#include <evpp/libevent.h>
#include <evpp/timestamp.h>
#include <evpp/event_loop_thread.h>

#include <evpp/httpc/request.h>
#include <evpp/httpc/conn.h>
#include <evpp/httpc/response.h>

#include "evpp/http/service.h"
#include "evpp/http/context.h"
#include "evpp/http/http_server.h"

static bool g_stopping = false;

static void DefaultRequestHandler(evpp::EventLoop* loop, const evpp::http::ContextPtr& ctx, const evpp::http::HTTPSendResponseCallback& cb) {
//std::cout << __func__ << " called ...\n";
std::stringstream oss;
oss << "func=" << __FUNCTION__ << "\n"
<< " ip=" << ctx->remote_ip() << "\n"
<< " uri=" << ctx->uri() << "\n"
<< " body=" << ctx->body().ToString() << "\n";

if (ctx->uri().find("stop") != std::string::npos) {
g_stopping = true;
}

cb(oss.str());
}

static void RequestHandlerHTTPClientRetry(evpp::EventLoop* loop, const evpp::http::ContextPtr& ctx, const evpp::http::HTTPSendResponseCallback& cb) {
std::stringstream oss;
oss << "func=" << __FUNCTION__ << " OK"
<< " ip=" << ctx->remote_ip() << "\n"
<< " uri=" << ctx->uri() << "\n"
<< " body=" << ctx->body().ToString() << "\n";
static std::atomic<int> i(0);
if (i++ == 0) {
std::this_thread::sleep_for(std::chrono::seconds(3));
}
cb(oss.str());
}

namespace {

static std::vector<int> g_listening_port = { 49000, 49001 };

static std::string GetHttpServerURL() {
static int i = 0;
std::ostringstream oss;
oss << "http://127.0.0.1:" << g_listening_port[(i++ % g_listening_port.size())];
return oss.str();
}

void testRequestHandlerRetry(evpp::EventLoop* loop, int* finished) {
std::string uri = "/retry";
std::string url = GetHttpServerURL() + uri;
auto r = new evpp::httpc::Request(loop, url, "", evpp::Duration(2.0));
r->set_retry_number(3);
auto f = [r, finished](const std::shared_ptr<evpp::httpc::Response>& response) {
std::string result = response->body().ToString();
H_TEST_ASSERT(!result.empty());
H_TEST_ASSERT(result.find("uri=/retry") != std::string::npos);
*finished += 1;
delete r;
};

r->Execute(f);
}

void testStop(evpp::EventLoop* loop, int* finished) {
std::string uri = "/mod/stop";
std::string url = GetHttpServerURL() + uri;
auto r = new evpp::httpc::Request(loop, url, "", evpp::Duration(10.0));
auto f = [r, finished](const std::shared_ptr<evpp::httpc::Response>& response) {
std::string result = response->body().ToString();
H_TEST_ASSERT(!result.empty());
H_TEST_ASSERT(result.find("uri=/mod/stop") != std::string::npos);
H_TEST_ASSERT(result.find("func=DefaultRequestHandler") != std::string::npos);
*finished += 1;
delete r;
};

r->Execute(f);
}

static void TestHTTPClientRetry() {
evpp::EventLoopThread t;
t.Start(true);
int finished = 0;
testRequestHandlerRetry(t.loop(), &finished);
testStop(t.loop(), &finished);

while (true) {
usleep(10);

if (finished == 2) {
break;
}
}

t.Stop(true);
}

void testRequestHandlerRetryFailed(evpp::EventLoop* loop, int* finished) {
std::string uri = "/retry";
std::string url = GetHttpServerURL() + uri;
auto r = new evpp::httpc::Request(loop, url, "", evpp::Duration(2.0));
r->set_retry_number(3);
auto f = [r, finished](const std::shared_ptr<evpp::httpc::Response>& response) {
std::string result = response->body().ToString();
H_TEST_ASSERT(result.empty());
*finished += 1;
delete r;
};

r->Execute(f);
}


static void TestHTTPClientRetryFailed() {
evpp::EventLoopThread t;
t.Start(true);
int finished = 0;
testRequestHandlerRetryFailed(t.loop(), &finished);

while (true) {
usleep(10);

if (finished == 1) {
break;
}
}

t.Stop(true);
}
}

TEST_UNIT(testHTTPClientRetry) {
for (int i = 0; i < 1; ++i) {
LOG_INFO << "Running testHTTPClientRetry i=" << i;
evpp::http::Server ph(i);
ph.RegisterDefaultHandler(&DefaultRequestHandler);
ph.RegisterHandler("/retry", &RequestHandlerHTTPClientRetry);
bool r = ph.Init(g_listening_port) && ph.Start();
H_TEST_ASSERT(r);
TestHTTPClientRetry();
ph.Stop();
usleep(1000 * 1000); // sleep a while to release the listening address and port
}
}

TEST_UNIT(testHTTPClientRetryFailed) {
for (int i = 0; i < 1; ++i) {
LOG_INFO << "Running testHTTPClientRetry i=" << i;
TestHTTPClientRetryFailed();
usleep(1000 * 1000); // sleep a while to release the listening address and port
}
}
47 changes: 0 additions & 47 deletions test/http_server_test.cc
Expand Up @@ -188,22 +188,6 @@ void testRequestHandler909(evpp::EventLoop* loop, int* finished) {
r->Execute(f);
}

void testRequestHandlerRetry(evpp::EventLoop* loop, int* finished) {
std::string uri = "/retry";
std::string url = GetHttpServerURL() + uri;
auto r = new evpp::httpc::Request(loop, url, "", evpp::Duration(2.0));
r->set_retry_number(3);
auto f = [r, finished](const std::shared_ptr<evpp::httpc::Response>& response) {
std::string result = response->body().ToString();
H_TEST_ASSERT(!result.empty());
H_TEST_ASSERT(result.find("uri=/retry") != std::string::npos);
*finished += 1;
delete r;
};

r->Execute(f);
}

void testStop(evpp::EventLoop* loop, int* finished) {
std::string uri = "/mod/stop";
std::string url = GetHttpServerURL() + uri;
Expand Down Expand Up @@ -261,23 +245,6 @@ static void Test909() {
t.Stop(true);
}

static void TestHTTPClientRetry() {
evpp::EventLoopThread t;
t.Start(true);
int finished = 0;
testRequestHandlerRetry(t.loop(), &finished);
testStop(t.loop(), &finished);

while (true) {
usleep(10);

if (finished == 2) {
break;
}
}

t.Stop(true);
}
}


Expand Down Expand Up @@ -311,20 +278,6 @@ TEST_UNIT(testHTTPServer909) {
}
}

TEST_UNIT(testHTTPClientRetry) {
for (int i = 0; i < 1; ++i) {
LOG_INFO << "Running testHTTPClientRetry i=" << i;
evpp::http::Server ph(i);
ph.RegisterDefaultHandler(&DefaultRequestHandler);
ph.RegisterHandler("/retry", &RequestHandlerHTTPClientRetry);
bool r = ph.Init(g_listening_port) && ph.Start();
H_TEST_ASSERT(r);
TestHTTPClientRetry();
ph.Stop();
usleep(1000 * 1000); // sleep a while to release the listening address and port
}
}

TEST_UNIT(testFindClientIP) {
struct TestCase {
std::string uri;
Expand Down
1 change: 1 addition & 0 deletions vsprojects/libevpp-test.vcxproj
Expand Up @@ -120,6 +120,7 @@
<ExcludedFromBuild Condition="'$(Configuration)|$(Platform)'=='Debug|Win32'">false</ExcludedFromBuild>
<ExcludedFromBuild Condition="'$(Configuration)|$(Platform)'=='Release|Win32'">false</ExcludedFromBuild>
</ClCompile>
<ClCompile Include="..\test\http_client_test.cc" />
<ClCompile Include="..\test\http_server_test.cc">
<ExcludedFromBuild Condition="'$(Configuration)|$(Platform)'=='Debug|Win32'">false</ExcludedFromBuild>
<ExcludedFromBuild Condition="'$(Configuration)|$(Platform)'=='Release|Win32'">false</ExcludedFromBuild>
Expand Down

0 comments on commit 0c94eb2

Please sign in to comment.