Skip to content

Fixes remaining memory leaks with nexthop strategy unit tests #6816

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
bryancall merged 1 commit into from
May 21, 2020
Merged

Fixes remaining memory leaks with nexthop strategy unit tests #6816

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions proxy/http/remap/NextHopStrategyFactory.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -229,6 +229,7 @@ NextHopStrategyFactory::loadConfigFile(const std::string fileName, std::stringst
}
}
}
closedir(dir);
} else {
std::ifstream file(fileName);
if (file.is_open()) {
Expand Down
42 changes: 21 additions & 21 deletions proxy/http/remap/unit-tests/nexthop_test_stubs.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,32 +32,32 @@
#include "HttpTransact.h"

void
br(HttpRequestData *h, const char *os_hostname, sockaddr const *dest_ip)
br_destroy(HttpRequestData &h)
{
HdrHeap *heap = new_HdrHeap(HdrHeap::DEFAULT_SIZE + 64);
h->hdr = new HTTPHdr();
h->hdr->create(HTTP_TYPE_REQUEST, heap);
h->hostname_str = ats_strdup(os_hostname);
h->xact_start = time(nullptr);
ink_zero(h->src_ip);
ink_zero(h->dest_ip);
ats_ip_copy(&h->dest_ip.sa, dest_ip);
h->incoming_port = 80;
h->api_info = new HttpApiInfo();
delete h.hdr;
delete h.api_info;
ats_free(h.hostname_str);
}

void
br_reinit(HttpRequestData *h)
build_request(HttpRequestData &h, const char *os_hostname)
{
if (h->hdr) {
ats_free(h->hdr);
ats_free(h->hostname_str);
if (h->hdr) {
delete h->hdr;
}
if (h->api_info) {
delete h->api_info;
}
HdrHeap *heap = nullptr;

if (h.hdr == nullptr) {
h.hdr = new HTTPHdr();
h.hdr->create(HTTP_TYPE_REQUEST, heap);
h.xact_start = time(nullptr);
h.incoming_port = 80;
ink_zero(h.src_ip);
ink_zero(h.dest_ip);
}
if (h.hostname_str != nullptr) {
ats_free(h.hostname_str);
h.hostname_str = ats_strdup(os_hostname);
}
if (h.api_info == nullptr) {
h.api_info = new HttpApiInfo();
}
}

Expand Down
4 changes: 4 additions & 0 deletions proxy/http/remap/unit-tests/nexthop_test_stubs.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,11 @@ extern "C" {
#define NH_Note(fmt, ...) PrintToStdErr("%s:%d:%s() " fmt "\n", __FILE__, __LINE__, __func__, ##__VA_ARGS__)
#define NH_Warn(fmt, ...) PrintToStdErr("%s:%d:%s() " fmt "\n", __FILE__, __LINE__, __func__, ##__VA_ARGS__)

class HttpRequestData;

void PrintToStdErr(const char *fmt, ...);
void br_destroy(HttpRequestData &h);
void build_request(HttpRequestData &h, const char *os_hostname);

#ifdef __cplusplus
}
Expand Down
56 changes: 31 additions & 25 deletions proxy/http/remap/unit-tests/test_NextHopConsistentHash.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ extern int cmd_disable_pfreelist;

SCENARIO("Testing NextHopConsistentHash class, using policy 'consistent_hash'", "[NextHopConsistentHash]")
{
// We need this to build a HdrHeap object in br();
// We need this to build a HdrHeap object in build_request();
// No thread setup, forbid use of thread local allocators.
cmd_disable_pfreelist = true;
// Get all of the HTTP WKS items populated.
Expand Down Expand Up @@ -92,7 +92,7 @@ SCENARIO("Testing NextHopConsistentHash class, using policy 'consistent_hash'",
REQUIRE(strategy != nullptr);

// first request.
br(&request, "rabbit.net");
build_request(request, "rabbit.net");
result.reset();
strategy->findNextHop(10001, result, request, fail_threshold, retry_time);

Expand All @@ -106,7 +106,7 @@ SCENARIO("Testing NextHopConsistentHash class, using policy 'consistent_hash'",

// second request - reusing the ParentResult from the last request
// simulating a failure triggers a search for another parent, not firstcall.
br(&request, "rabbit.net");
build_request(request, "rabbit.net");
strategy->findNextHop(10002, result, request, fail_threshold, retry_time);

CHECK(result.result == ParentResultType::PARENT_SPECIFIED);
Expand All @@ -117,7 +117,7 @@ SCENARIO("Testing NextHopConsistentHash class, using policy 'consistent_hash'",

// third request - reusing the ParentResult from the last request
// simulating a failure triggers a search for another parent, not firstcall.
br(&request, "rabbit.net");
build_request(request, "rabbit.net");
strategy->findNextHop(10003, result, request, fail_threshold, retry_time);

CHECK(result.result == ParentResultType::PARENT_SPECIFIED);
Expand All @@ -128,7 +128,7 @@ SCENARIO("Testing NextHopConsistentHash class, using policy 'consistent_hash'",

// fourth request - reusing the ParentResult from the last request
// simulating a failure triggers a search for another parent, not firstcall.
br(&request, "rabbit.net");
build_request(request, "rabbit.net");
strategy->findNextHop(10004, result, request, fail_threshold, retry_time);

CHECK(result.result == ParentResultType::PARENT_SPECIFIED);
Expand All @@ -139,7 +139,7 @@ SCENARIO("Testing NextHopConsistentHash class, using policy 'consistent_hash'",

// fifth request - reusing the ParentResult from the last request
// simulating a failure triggers a search for another parent, not firstcall.
br(&request, "rabbit.net");
build_request(request, "rabbit.net");
strategy->findNextHop(10005, result, request, fail_threshold, retry_time);

CHECK(result.result == ParentResultType::PARENT_SPECIFIED);
Expand All @@ -149,7 +149,7 @@ SCENARIO("Testing NextHopConsistentHash class, using policy 'consistent_hash'",
strategy->markNextHopDown(10005, result, 1, fail_threshold);
// sixth request - reusing the ParentResult from the last request
// simulating a failure triggers a search for another parent, not firstcall.
br(&request, "rabbit.net");
build_request(request, "rabbit.net");
strategy->findNextHop(10006, result, request, fail_threshold, retry_time);

CHECK(result.result == ParentResultType::PARENT_SPECIFIED);
Expand All @@ -159,7 +159,7 @@ SCENARIO("Testing NextHopConsistentHash class, using policy 'consistent_hash'",
strategy->markNextHopDown(10006, result, 1, fail_threshold);
// seventh request - reusing the ParentResult from the last request
// simulating a failure triggers a search for another parent, not firstcall.
br(&request, "rabbit.net");
build_request(request, "rabbit.net");
strategy->findNextHop(10007, result, request, fail_threshold, retry_time);

CHECK(result.result == ParentResultType::PARENT_DIRECT);
Expand All @@ -170,18 +170,20 @@ SCENARIO("Testing NextHopConsistentHash class, using policy 'consistent_hash'",

// eighth request - reusing the ParentResult from the last request
// simulating a failure triggers a search for another parent, not firstcall.
br(&request, "rabbit.net");
build_request(request, "rabbit.net");
strategy->findNextHop(10008, result, request, fail_threshold, retry_time, now);
CHECK(result.result == ParentResultType::PARENT_SPECIFIED);
CHECK(strcmp(result.hostname, "q2.bar.com") == 0);
}
// free up request resources.
br_destroy(request);
}
}
}

SCENARIO("Testing NextHopConsistentHash class (all firstcalls), using policy 'consistent_hash'", "[NextHopConsistentHash]")
{
// We need this to build a HdrHeap object in br();
// We need this to build a HdrHeap object in build_request();
// No thread setup, forbid use of thread local allocators.
cmd_disable_pfreelist = true;
// Get all of the HTTP WKS items populated.
Expand Down Expand Up @@ -224,7 +226,7 @@ SCENARIO("Testing NextHopConsistentHash class (all firstcalls), using policy 'co
REQUIRE(strategy != nullptr);

// first request.
br(&request, "rabbit.net");
build_request(request, "rabbit.net");
result.reset();
strategy->findNextHop(20001, result, request, fail_threshold, retry_time);
CHECK(result.result == ParentResultType::PARENT_SPECIFIED);
Expand All @@ -233,7 +235,7 @@ SCENARIO("Testing NextHopConsistentHash class (all firstcalls), using policy 'co
// mark down p1.foo.com
strategy->markNextHopDown(20001, result, 1, fail_threshold);
// second request
br(&request, "rabbit.net");
build_request(request, "rabbit.net");
result.reset();
strategy->findNextHop(20002, result, request, fail_threshold, retry_time);
CHECK(result.result == ParentResultType::PARENT_SPECIFIED);
Expand All @@ -243,7 +245,7 @@ SCENARIO("Testing NextHopConsistentHash class (all firstcalls), using policy 'co
strategy->markNextHopDown(20002, result, 1, fail_threshold);

// third request
br(&request, "rabbit.net");
build_request(request, "rabbit.net");
result.reset();
strategy->findNextHop(20003, result, request, fail_threshold, retry_time);
CHECK(result.result == ParentResultType::PARENT_SPECIFIED);
Expand All @@ -253,7 +255,7 @@ SCENARIO("Testing NextHopConsistentHash class (all firstcalls), using policy 'co
strategy->markNextHopDown(20003, result, 1, fail_threshold);

// fourth request
br(&request, "rabbit.net");
build_request(request, "rabbit.net");
result.reset();
strategy->findNextHop(20004, result, request, fail_threshold, retry_time);
CHECK(result.result == ParentResultType::PARENT_SPECIFIED);
Expand All @@ -263,27 +265,29 @@ SCENARIO("Testing NextHopConsistentHash class (all firstcalls), using policy 'co
strategy->markNextHopDown(20004, result, 1, fail_threshold);

// fifth request
br(&request, "rabbit.net/asset1");
build_request(request, "rabbit.net/asset1");
result.reset();
strategy->findNextHop(20005, result, request, fail_threshold, retry_time);
CHECK(result.result == ParentResultType::PARENT_SPECIFIED);
CHECK(strcmp(result.hostname, "q1.bar.com") == 0);

// sixth request - wait and p1 should now become available
time_t now = time(nullptr) + 5;
br(&request, "rabbit.net");
build_request(request, "rabbit.net");
result.reset();
strategy->findNextHop(20006, result, request, fail_threshold, retry_time, now);
CHECK(result.result == ParentResultType::PARENT_SPECIFIED);
CHECK(strcmp(result.hostname, "p1.foo.com") == 0);
}
// free up request resources.
br_destroy(request);
}
}
}

SCENARIO("Testing NextHopConsistentHash class (alternating rings), using policy 'consistent_hash'", "[NextHopConsistentHash]")
{
// We need this to build a HdrHeap object in br();
// We need this to build a HdrHeap object in build_request();
// No thread setup, forbid use of thread local allocators.
cmd_disable_pfreelist = true;
// Get all of the HTTP WKS items populated.
Expand Down Expand Up @@ -321,7 +325,7 @@ SCENARIO("Testing NextHopConsistentHash class (alternating rings), using policy
REQUIRE(strategy != nullptr);

// first request.
br(&request, "bunny.net/asset1");
build_request(request, "bunny.net/asset1");
result.reset();
strategy->findNextHop(30001, result, request, fail_threshold, retry_time);
CHECK(result.result == ParentResultType::PARENT_SPECIFIED);
Expand All @@ -331,7 +335,7 @@ SCENARIO("Testing NextHopConsistentHash class (alternating rings), using policy
strategy->markNextHopDown(30001, result, 1, fail_threshold);

// second request
br(&request, "bunny.net.net/asset1");
build_request(request, "bunny.net.net/asset1");
strategy->findNextHop(30002, result, request, fail_threshold, retry_time);
CHECK(result.result == ParentResultType::PARENT_SPECIFIED);
CHECK(strcmp(result.hostname, "c3.bar.com") == 0);
Expand All @@ -340,7 +344,7 @@ SCENARIO("Testing NextHopConsistentHash class (alternating rings), using policy
strategy->markNextHopDown(30002, result, 1, fail_threshold);

// third request
br(&request, "bunny.net/asset2");
build_request(request, "bunny.net/asset2");
result.reset();
strategy->findNextHop(30003, result, request, fail_threshold, retry_time);
CHECK(result.result == ParentResultType::PARENT_SPECIFIED);
Expand All @@ -349,15 +353,15 @@ SCENARIO("Testing NextHopConsistentHash class (alternating rings), using policy
// just mark it down and retry request
strategy->markNextHopDown(30003, result, 1, fail_threshold);
// fourth request
br(&request, "bunny.net/asset2");
build_request(request, "bunny.net/asset2");
strategy->findNextHop(30004, result, request, fail_threshold, retry_time);
CHECK(result.result == ParentResultType::PARENT_SPECIFIED);
CHECK(strcmp(result.hostname, "c1.foo.com") == 0);

// mark it down
strategy->markNextHopDown(30004, result, 1, fail_threshold);
// fifth request - new request
br(&request, "bunny.net/asset3");
build_request(request, "bunny.net/asset3");
result.reset();
strategy->findNextHop(30005, result, request, fail_threshold, retry_time);
CHECK(result.result == ParentResultType::PARENT_SPECIFIED);
Expand All @@ -366,7 +370,7 @@ SCENARIO("Testing NextHopConsistentHash class (alternating rings), using policy
// mark it down and retry
strategy->markNextHopDown(30005, result, 1, fail_threshold);
// sixth request
br(&request, "bunny.net/asset3");
build_request(request, "bunny.net/asset3");
result.reset();
strategy->findNextHop(30006, result, request, fail_threshold, retry_time);
CHECK(result.result == ParentResultType::PARENT_SPECIFIED);
Expand All @@ -375,20 +379,22 @@ SCENARIO("Testing NextHopConsistentHash class (alternating rings), using policy
// mark it down
strategy->markNextHopDown(30006, result, 1, fail_threshold);
// seventh request - new request with all hosts down and go_direct is false.
br(&request, "bunny.net/asset4");
build_request(request, "bunny.net/asset4");
result.reset();
strategy->findNextHop(30007, result, request, fail_threshold, retry_time);
CHECK(result.result == ParentResultType::PARENT_FAIL);
CHECK(result.hostname == nullptr);

// eighth request - retry after waiting for the retry window to expire.
time_t now = time(nullptr) + 5;
br(&request, "bunny.net/asset4");
build_request(request, "bunny.net/asset4");
result.reset();
strategy->findNextHop(30008, result, request, fail_threshold, retry_time, now);
CHECK(result.result == ParentResultType::PARENT_SPECIFIED);
CHECK(strcmp(result.hostname, "c2.foo.com") == 0);
}
// free up request resources.
br_destroy(request);
}
}
}