From 334340da51dcfe51bc6cc049983a0e6eb4c0ed0f Mon Sep 17 00:00:00 2001 From: Azat Khuzhin Date: Sat, 12 Mar 2016 18:50:41 +0300 Subject: [PATCH 1/7] test/http: request cancellation with resolving/{conn,write}-timeouts in progress This patch adds 8 new tests: - http/cancel - http/cancel_by_host - http/cancel_by_host_no_ns - http/cancel_by_host_inactive_server - http/cancel_inactive_server - http/cancel_by_host_no_ns_inactive_server - http/cancel_by_host_server_timeout - http/cancel_server_timeout - http/cancel_by_host_no_ns_server_timeout This patches not 100% for http layer, but more for be_sock, but it was simpler to add them here, plus it also shows some bugs with fd leaking in http layer. Right now we have next picture (we can also play with timeouts/attempts for evdns to make tests fail, IOW to track the failures even without valgrind): $ valgrind --leak-check=full --show-reachable=yes --track-fds=yes --error-exitcode=1 regress --no-fork http/cancel.. http/cancel: OK http/cancel_by_host: OK http/cancel_by_host_no_ns: [msg] Nameserver 127.0.0.1:42489 has failed: request timed out. [msg] All nameservers have failed OK http/cancel_by_host_inactive_server: OK http/cancel_inactive_server: OK http/cancel_by_host_no_ns_inactive_server: [msg] Nameserver 127.0.0.1:51370 has failed: request timed out. [msg] All nameservers have failed OK http/cancel_by_host_server_timeout: OK http/cancel_server_timeout: OK http/cancel_by_host_no_ns_server_timeout: [msg] Nameserver 127.0.0.1:45054 has failed: request timed out. [msg] All nameservers have failed OK 9 tests ok. (0 skipped) ==3202== ==3202== FILE DESCRIPTORS: 2309 open at exit. ... ==8403== HEAP SUMMARY: ==8403== in use at exit: 1,104 bytes in 5 blocks ==8403== total heap usage: 10,916 allocs, 10,911 frees, 1,458,818 bytes allocated ==8403== ==8403== 40 bytes in 1 blocks are indirectly lost in loss record 1 of 5 ==8403== at 0x4C2BBD5: calloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) ==8403== by 0x4AAD2D: event_mm_calloc_ (event.c:3459) ==8403== by 0x498E48: evbuffer_add_cb (buffer.c:3309) ==8403== by 0x4A0DE2: bufferevent_socket_new (bufferevent_sock.c:366) ==8403== by 0x4BF8BA: evhttp_connection_base_bufferevent_new (http.c:2369) ==8403== by 0x4BFA6A: evhttp_connection_base_new (http.c:2421) ==8403== by 0x460CFC: http_cancel_test (regress_http.c:1413) ==8403== by 0x490965: testcase_run_bare_ (tinytest.c:105) ==8403== by 0x490C47: testcase_run_one (tinytest.c:252) ==8403== by 0x491586: tinytest_main (tinytest.c:434) ==8403== by 0x47DFCD: main (regress_main.c:461) ==8403== ==8403== 136 bytes in 1 blocks are indirectly lost in loss record 2 of 5 ==8403== at 0x4C2BBD5: calloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) ==8403== by 0x4AAD2D: event_mm_calloc_ (event.c:3459) ==8403== by 0x491EDD: evbuffer_new (buffer.c:365) ==8403== by 0x49A0AB: bufferevent_init_common_ (bufferevent.c:300) ==8403== by 0x4A0D31: bufferevent_socket_new (bufferevent_sock.c:353) ==8403== by 0x4BF8BA: evhttp_connection_base_bufferevent_new (http.c:2369) ==8403== by 0x4BFA6A: evhttp_connection_base_new (http.c:2421) ==8403== by 0x460CFC: http_cancel_test (regress_http.c:1413) ==8403== by 0x490965: testcase_run_bare_ (tinytest.c:105) ==8403== by 0x490C47: testcase_run_one (tinytest.c:252) ==8403== by 0x491586: tinytest_main (tinytest.c:434) ==8403== by 0x47DFCD: main (regress_main.c:461) ==8403== ==8403== 136 bytes in 1 blocks are indirectly lost in loss record 3 of 5 ==8403== at 0x4C2BBD5: calloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) ==8403== by 0x4AAD2D: event_mm_calloc_ (event.c:3459) ==8403== by 0x491EDD: evbuffer_new (buffer.c:365) ==8403== by 0x49A0E8: bufferevent_init_common_ (bufferevent.c:305) ==8403== by 0x4A0D31: bufferevent_socket_new (bufferevent_sock.c:353) ==8403== by 0x4BF8BA: evhttp_connection_base_bufferevent_new (http.c:2369) ==8403== by 0x4BFA6A: evhttp_connection_base_new (http.c:2421) ==8403== by 0x460CFC: http_cancel_test (regress_http.c:1413) ==8403== by 0x490965: testcase_run_bare_ (tinytest.c:105) ==8403== by 0x490C47: testcase_run_one (tinytest.c:252) ==8403== by 0x491586: tinytest_main (tinytest.c:434) ==8403== by 0x47DFCD: main (regress_main.c:461) ==8403== ==8403== 528 bytes in 1 blocks are indirectly lost in loss record 4 of 5 ==8403== at 0x4C2BBD5: calloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) ==8403== by 0x4AAD2D: event_mm_calloc_ (event.c:3459) ==8403== by 0x4A0D02: bufferevent_socket_new (bufferevent_sock.c:350) ==8403== by 0x4BF8BA: evhttp_connection_base_bufferevent_new (http.c:2369) ==8403== by 0x4BFA6A: evhttp_connection_base_new (http.c:2421) ==8403== by 0x460CFC: http_cancel_test (regress_http.c:1413) ==8403== by 0x490965: testcase_run_bare_ (tinytest.c:105) ==8403== by 0x490C47: testcase_run_one (tinytest.c:252) ==8403== by 0x491586: tinytest_main (tinytest.c:434) ==8403== by 0x47DFCD: main (regress_main.c:461) ==8403== ==8403== 1,104 (264 direct, 840 indirect) bytes in 1 blocks are definitely lost in loss record 5 of 5 ==8403== at 0x4C2BBD5: calloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) ==8403== by 0x4AAD2D: event_mm_calloc_ (event.c:3459) ==8403== by 0x4D0326: evdns_getaddrinfo (evdns.c:4682) ==8403== by 0x4B1213: evutil_getaddrinfo_async_ (evutil.c:1568) ==8403== by 0x4A1255: bufferevent_socket_connect_hostname (bufferevent_sock.c:517) ==8403== by 0x4C00B6: evhttp_connection_connect_ (http.c:2582) ==8403== by 0x4C02B8: evhttp_make_request (http.c:2637) ==8403== by 0x4614EC: http_cancel_test (regress_http.c:1496) ==8403== by 0x490965: testcase_run_bare_ (tinytest.c:105) ==8403== by 0x490C47: testcase_run_one (tinytest.c:252) ==8403== by 0x491586: tinytest_main (tinytest.c:434) ==8403== by 0x47DFCD: main (regress_main.c:461) ==8403== ==8403== LEAK SUMMARY: ==8403== definitely lost: 264 bytes in 1 blocks ==8403== indirectly lost: 840 bytes in 4 blocks ==8403== possibly lost: 0 bytes in 0 blocks ==8403== still reachable: 0 bytes in 0 blocks ==8403== suppressed: 0 bytes in 0 blocks --- test/regress_http.c | 185 ++++++++++++++++++++++++++++++++++++++------ 1 file changed, 163 insertions(+), 22 deletions(-) diff --git a/test/regress_http.c b/test/regress_http.c index 22e71add..9cf9ee5a 100644 --- a/test/regress_http.c +++ b/test/regress_http.c @@ -1268,6 +1268,21 @@ http_request_never_call(struct evhttp_request *req, void *arg) fprintf(stdout, "FAILED\n"); exit(1); } +static void +http_failed_request_done(struct evhttp_request *req, void *arg) +{ + tt_assert(!req); +end: + event_base_loopexit(arg, NULL); +} +static void +http_timed_out_request_done(struct evhttp_request *req, void *arg) +{ + tt_assert(req); + tt_int_op(evhttp_request_get_response_code(req), !=, HTTP_OK); +end: + event_base_loopexit(arg, NULL); +} static void http_request_error_cb_with_cancel(enum evhttp_request_error error, void *arg) @@ -1293,7 +1308,58 @@ http_do_cancel(evutil_socket_t fd, short what, void *arg) evhttp_cancel_request(req); ++test_ok; } +static void +http_no_write(struct evbuffer *buffer, const struct evbuffer_cb_info *info, void *arg) +{ + fprintf(stdout, "FAILED\n"); + exit(1); +} +static void +http_free_evcons(struct evhttp_connection **evcons) +{ + if (!evcons) + return; + struct evhttp_connection *evcon, **orig = evcons; + while ((evcon = *evcons++)) { + evhttp_connection_free(evcon); + } + free(orig); +} +/** fill the backlog to force server drop packages for timeouts */ +static struct evhttp_connection ** +http_fill_backlog(struct event_base *base, int port) +{ +#define BACKLOG_SIZE 256 + struct evhttp_connection **evcon = malloc(sizeof(*evcon) * (BACKLOG_SIZE + 1)); + int i; + + for (i = 0; i < BACKLOG_SIZE; ++i) { + struct evhttp_request *req; + + evcon[i] = evhttp_connection_base_new(base, NULL, "127.0.0.1", port); + tt_assert(evcon[i]); + evhttp_connection_set_timeout(evcon[i], 5); + + req = evhttp_request_new(http_request_never_call, NULL); + tt_assert(req); + tt_int_op(evhttp_make_request(evcon[i], req, EVHTTP_REQ_GET, "/delay"), !=, -1); + } + evcon[i] = NULL; + + return evcon; + end: + fprintf(stderr, "Couldn't fill the backlog"); + return NULL; +} + +enum http_cancel_test_type { + BASIC = 1, + BY_HOST = 2, + NO_NS = 4, + INACTIVE_SERVER = 8, + SERVER_TIMEOUT = 16, +}; static void http_cancel_test(void *arg) { @@ -1301,7 +1367,33 @@ http_cancel_test(void *arg) ev_uint16_t port = 0; struct evhttp_connection *evcon = NULL; struct evhttp_request *req = NULL; + struct bufferevent *bufev = NULL; struct timeval tv; + struct evdns_base *dns_base = NULL; + ev_uint16_t portnum = 0; + char address[64]; + struct evhttp *inactive_http = NULL; + struct event_base *inactive_base = NULL; + struct evhttp_connection **evcons = NULL; + + enum http_cancel_test_type type; + type = (enum http_cancel_test_type)data->setup_data; + + if (type & BY_HOST) { + tt_assert(regress_dnsserver(data->base, &portnum, search_table)); + + dns_base = evdns_base_new(data->base, 0/* init name servers */); + tt_assert(dns_base); + + /** XXX: Hack the port to make timeout after resolving */ + if (type & NO_NS) + ++portnum; + + evutil_snprintf(address, sizeof(address), "127.0.0.1:%d", portnum); + evdns_base_nameserver_ip_add(dns_base, address); + evdns_base_set_option(dns_base, "timeout:", "3"); + evdns_base_set_option(dns_base, "attempts:", "1"); + } exit_base = data->base; @@ -1309,9 +1401,29 @@ http_cancel_test(void *arg) http = http_setup(&port, data->base, 0); - evcon = evhttp_connection_base_new(data->base, NULL, "127.0.0.1", port); + if (type & INACTIVE_SERVER) { + port = 0; + inactive_base = event_base_new(); + inactive_http = http_setup(&port, inactive_base, 0); + } + + if (type & SERVER_TIMEOUT) + evcons = http_fill_backlog(inactive_base ?: data->base, port); + + evcon = evhttp_connection_base_new( + data->base, dns_base, + type & BY_HOST ? "localhost" : "127.0.0.1", + port); + if (type & INACTIVE_SERVER) + evhttp_connection_set_timeout(evcon, 5); tt_assert(evcon); + bufev = evhttp_connection_get_bufferevent(evcon); + /* Guarantee that we stack in connect() not after waiting EV_READ after + * write() */ + if (type & SERVER_TIMEOUT) + evbuffer_add_cb(bufferevent_get_output(bufev), http_no_write, NULL); + /* * At this point, we want to schedule a request to the HTTP * server using our make request method. @@ -1335,12 +1447,24 @@ http_cancel_test(void *arg) event_base_dispatch(data->base); - tt_int_op(test_ok, ==, 3); + if (type & NO_NS || type & INACTIVE_SERVER) + tt_int_op(test_ok, ==, 2); /** no servers responses */ + else + tt_int_op(test_ok, ==, 3); /* try to make another request over the same connection */ test_ok = 0; - req = evhttp_request_new(http_request_done, (void*) BASIC_REQUEST_BODY); + http_free_evcons(evcons); + if (type & SERVER_TIMEOUT) + evcons = http_fill_backlog(inactive_base ?: data->base, port); + + if (!(type & NO_NS) && (type & SERVER_TIMEOUT)) + req = evhttp_request_new(http_timed_out_request_done, data->base); + else if ((type & INACTIVE_SERVER) || (type & NO_NS)) + req = evhttp_request_new(http_failed_request_done, data->base); + else + req = evhttp_request_new(http_request_done, (void*) BASIC_REQUEST_BODY); /* Add the information that we care about */ evhttp_add_header(evhttp_request_get_output_headers(req), "Host", "somehost"); @@ -1354,7 +1478,16 @@ http_cancel_test(void *arg) /* make another request: request empty reply */ test_ok = 0; - req = evhttp_request_new(http_request_empty_done, data->base); + http_free_evcons(evcons); + if (type & SERVER_TIMEOUT) + evcons = http_fill_backlog(inactive_base ?: data->base, port); + + if (!(type & NO_NS) && (type & SERVER_TIMEOUT)) + req = evhttp_request_new(http_timed_out_request_done, data->base); + else if ((type & INACTIVE_SERVER) || (type & NO_NS)) + req = evhttp_request_new(http_failed_request_done, data->base); + else + req = evhttp_request_new(http_request_empty_done, data->base); /* Add the information that we care about */ evhttp_add_header(evhttp_request_get_output_headers(req), "Empty", "itis"); @@ -1366,10 +1499,20 @@ http_cancel_test(void *arg) event_base_dispatch(data->base); end: + http_free_evcons(evcons); + if (bufev) + evbuffer_remove_cb(bufferevent_get_output(bufev), http_no_write, NULL); if (evcon) evhttp_connection_free(evcon); if (http) evhttp_free(http); + if (dns_base) + evdns_base_free(dns_base, 0); + regress_clean_dnsserver(); + if (inactive_http) + evhttp_free(inactive_http); + if (inactive_base) + event_base_free(inactive_base); } static void @@ -3770,13 +3913,6 @@ http_large_entity_test_done(struct evhttp_request *req, void *arg) end: event_base_loopexit(arg, NULL); } -static void -http_failed_request_done(struct evhttp_request *req, void *arg) -{ - tt_assert(!req); -end: - event_base_loopexit(arg, NULL); -} #ifndef WIN32 static void http_expectation_failed_done(struct evhttp_request *req, void *arg) @@ -3910,13 +4046,6 @@ static void http_data_length_constraints_test(void *arg) static void http_read_on_write_error_test(void *arg) { http_data_length_constraints_test_impl(arg, 1); } -static void -http_large_entity_non_lingering_test_done(struct evhttp_request *req, void *arg) -{ - tt_assert(!req); -end: - event_base_loopexit(arg, NULL); -} static void http_lingering_close_test_impl(void *arg, int lingering) { @@ -3951,7 +4080,7 @@ http_lingering_close_test_impl(void *arg, int lingering) if (lingering) cb = http_large_entity_test_done; else - cb = http_large_entity_non_lingering_test_done; + cb = http_failed_request_done; req = evhttp_request_new(cb, data->base); tt_assert(req); evhttp_add_header(evhttp_request_get_output_headers(req), "Host", "somehost"); @@ -4342,8 +4471,10 @@ http_request_own_test(void *arg) { #name, run_legacy_test_fn, TT_ISOLATED|TT_LEGACY, &legacy_setup, \ http_##name##_test } -#define HTTP(name) \ - { #name, http_##name##_test, TT_ISOLATED, &basic_setup, NULL } +#define HTTP_CAST_ARG(a) ((void *)(a)) +#define HTTP_N(title, name, arg) \ + { #title, http_##name##_test, TT_ISOLATED, &basic_setup, HTTP_CAST_ARG(arg) } +#define HTTP(name) HTTP_N(name, name, NULL) #define HTTPS(name) \ { "https_" #name, https_##name##_test, TT_ISOLATED, &basic_setup, NULL } @@ -4382,7 +4513,17 @@ struct testcase_t http_testcases[] = { { "uriencode", http_uriencode_test, 0, NULL, NULL }, HTTP(basic), HTTP(simple), - HTTP(cancel), + + HTTP_N(cancel, cancel, BASIC), + HTTP_N(cancel_by_host, cancel, BY_HOST), + HTTP_N(cancel_by_host_no_ns, cancel, BY_HOST | NO_NS), + HTTP_N(cancel_by_host_inactive_server, cancel, BY_HOST | INACTIVE_SERVER), + HTTP_N(cancel_inactive_server, cancel, INACTIVE_SERVER), + HTTP_N(cancel_by_host_no_ns_inactive_server, cancel, BY_HOST | NO_NS | INACTIVE_SERVER), + HTTP_N(cancel_by_host_server_timeout, cancel, BY_HOST | INACTIVE_SERVER | SERVER_TIMEOUT), + HTTP_N(cancel_server_timeout, cancel, INACTIVE_SERVER | SERVER_TIMEOUT), + HTTP_N(cancel_by_host_no_ns_server_timeout, cancel, BY_HOST | NO_NS | INACTIVE_SERVER | SERVER_TIMEOUT), + HTTP(virtual_host), HTTP(post), HTTP(put), From 8cbe65d5f44246ad3f7ab408d22e797e5b9e5b81 Mon Sep 17 00:00:00 2001 From: Azat Khuzhin Date: Mon, 21 Mar 2016 17:08:34 +0300 Subject: [PATCH 2/7] evdns: export cancel via callbacks in util (like async lib core/extra issues) --- evdns.c | 1 + evutil.c | 22 ++++++++++++++++++---- util-internal.h | 8 ++++++-- 3 files changed, 25 insertions(+), 6 deletions(-) diff --git a/evdns.c b/evdns.c index c4112330..152ba766 100644 --- a/evdns.c +++ b/evdns.c @@ -3908,6 +3908,7 @@ evdns_base_new(struct event_base *event_base, int flags) * functionality. We can't just call evdns_getaddrinfo directly or * else libevent-core will depend on libevent-extras. */ evutil_set_evdns_getaddrinfo_fn_(evdns_getaddrinfo); + evutil_set_evdns_getaddrinfo_cancel_fn_(evdns_getaddrinfo_cancel); base = mm_malloc(sizeof(struct evdns_base)); if (base == NULL) diff --git a/evutil.c b/evutil.c index 495bfcc0..06b4e948 100644 --- a/evutil.c +++ b/evutil.c @@ -1546,6 +1546,7 @@ evutil_freeaddrinfo(struct evutil_addrinfo *ai) } static evdns_getaddrinfo_fn evdns_getaddrinfo_impl = NULL; +static evdns_getaddrinfo_cancel_fn evdns_getaddrinfo_cancel_impl = NULL; void evutil_set_evdns_getaddrinfo_fn_(evdns_getaddrinfo_fn fn) @@ -1553,27 +1554,40 @@ evutil_set_evdns_getaddrinfo_fn_(evdns_getaddrinfo_fn fn) if (!evdns_getaddrinfo_impl) evdns_getaddrinfo_impl = fn; } +void +evutil_set_evdns_getaddrinfo_cancel_fn_(evdns_getaddrinfo_cancel_fn fn) +{ + if (!evdns_getaddrinfo_cancel_impl) + evdns_getaddrinfo_cancel_impl = fn; +} /* Internal helper function: act like evdns_getaddrinfo if dns_base is set; * otherwise do a blocking resolve and pass the result to the callback in the * way that evdns_getaddrinfo would. */ -int -evutil_getaddrinfo_async_(struct evdns_base *dns_base, +struct evdns_getaddrinfo_request *evutil_getaddrinfo_async_( + struct evdns_base *dns_base, const char *nodename, const char *servname, const struct evutil_addrinfo *hints_in, void (*cb)(int, struct evutil_addrinfo *, void *), void *arg) { if (dns_base && evdns_getaddrinfo_impl) { - evdns_getaddrinfo_impl( + return evdns_getaddrinfo_impl( dns_base, nodename, servname, hints_in, cb, arg); } else { struct evutil_addrinfo *ai=NULL; int err; err = evutil_getaddrinfo(nodename, servname, hints_in, &ai); cb(err, ai, arg); + return NULL; + } +} + +void evutil_getaddrinfo_cancel_async_(struct evdns_getaddrinfo_request *data) +{ + if (evdns_getaddrinfo_cancel_impl && data) { + evdns_getaddrinfo_cancel_impl(data); } - return 0; } const char * diff --git a/util-internal.h b/util-internal.h index ff6c5e48..b851e2ae 100644 --- a/util-internal.h +++ b/util-internal.h @@ -357,8 +357,10 @@ typedef struct evdns_getaddrinfo_request* (*evdns_getaddrinfo_fn)( const char *nodename, const char *servname, const struct evutil_addrinfo *hints_in, void (*cb)(int, struct evutil_addrinfo *, void *), void *arg); - void evutil_set_evdns_getaddrinfo_fn_(evdns_getaddrinfo_fn fn); +typedef void (*evdns_getaddrinfo_cancel_fn)( + struct evdns_getaddrinfo_request *req); +void evutil_set_evdns_getaddrinfo_cancel_fn_(evdns_getaddrinfo_cancel_fn fn); struct evutil_addrinfo *evutil_new_addrinfo_(struct sockaddr *sa, ev_socklen_t socklen, const struct evutil_addrinfo *hints); @@ -368,10 +370,12 @@ void evutil_adjust_hints_for_addrconfig_(struct evutil_addrinfo *hints); int evutil_getaddrinfo_common_(const char *nodename, const char *servname, struct evutil_addrinfo *hints, struct evutil_addrinfo **res, int *portnum); -int evutil_getaddrinfo_async_(struct evdns_base *dns_base, +struct evdns_getaddrinfo_request *evutil_getaddrinfo_async_( + struct evdns_base *dns_base, const char *nodename, const char *servname, const struct evutil_addrinfo *hints_in, void (*cb)(int, struct evutil_addrinfo *, void *), void *arg); +void evutil_getaddrinfo_cancel_async_(struct evdns_getaddrinfo_request *data); /** Return true iff sa is a looback address. (That is, it is 127.0.0.1/8, or * ::1). */ From 86dfd2ced1985ba7b2a2d4f48f2dc5d7a07ce0ab Mon Sep 17 00:00:00 2001 From: Azat Khuzhin Date: Mon, 21 Mar 2016 17:09:17 +0300 Subject: [PATCH 3/7] be_sock: cancel in-progress dns requests Before this patch we didn't have such functionality and this can cause some issues when we are cancelling HTTP request while after this we will get a response/timeout from the NS and in this case error_cb will be called and can damage newly started HTTP request. We could also have problems with connect, but we don't have them since we closes the fd in HTTP layer. This is not so good that this is done in be_sock, but it is internal, so we can change without pain. Plus I don't like that callback-via-evutil, but since we have event_extra we need do like that. And after this patch the following tests doesn't report leaks: $ valgrind --leak-check=full --show-reachable=yes --track-fds=yes --error-exitcode=1 regress --no-fork http/cancel.. ... ==10469== FILE DESCRIPTORS: 2309 open at exit. ... ==10469== HEAP SUMMARY: ==10469== in use at exit: 0 bytes in 0 blocks ==10469== total heap usage: 33,846 allocs, 33,846 frees, 4,617,651 bytes allocated ==10469== ==10469== All heap blocks were freed -- no leaks are possible v2: do under lock v3: reset dns request v4: ignore EVUTIL_EAI_CANCEL in regular callback --- bufferevent-internal.h | 2 ++ bufferevent_sock.c | 18 ++++++++++++++++-- 2 files changed, 18 insertions(+), 2 deletions(-) diff --git a/bufferevent-internal.h b/bufferevent-internal.h index d9d9e666..9960aefa 100644 --- a/bufferevent-internal.h +++ b/bufferevent-internal.h @@ -228,6 +228,8 @@ struct bufferevent_private { struct sockaddr_in6 in6; struct sockaddr_in in; } conn_address; + + struct evdns_getaddrinfo_request *dns_request; }; /** Possible operations for a control callback. */ diff --git a/bufferevent_sock.c b/bufferevent_sock.c index 643aa810..ab35f879 100644 --- a/bufferevent_sock.c +++ b/bufferevent_sock.c @@ -468,6 +468,13 @@ bufferevent_connect_getaddrinfo_cb(int result, struct evutil_addrinfo *ai, bufferevent_unsuspend_write_(bev, BEV_SUSPEND_LOOKUP); bufferevent_unsuspend_read_(bev, BEV_SUSPEND_LOOKUP); + bev_p->dns_request = NULL; + + if (result == EVUTIL_EAI_CANCEL) { + bev_p->dns_error = result; + bufferevent_decref_and_unlock_(bev); + return; + } if (result != 0) { bev_p->dns_error = result; bufferevent_run_eventcb_(bev, BEV_EVENT_ERROR, 0); @@ -514,8 +521,8 @@ bufferevent_socket_connect_hostname(struct bufferevent *bev, bufferevent_suspend_read_(bev, BEV_SUSPEND_LOOKUP); bufferevent_incref_(bev); - evutil_getaddrinfo_async_(evdns_base, hostname, portbuf, - &hint, bufferevent_connect_getaddrinfo_cb, bev); + bev_p->dns_request = evutil_getaddrinfo_async_(evdns_base, hostname, + portbuf, &hint, bufferevent_connect_getaddrinfo_cb, bev); BEV_UNLOCK(bev); return 0; @@ -603,6 +610,8 @@ be_socket_destruct(struct bufferevent *bufev) if ((bufev_p->options & BEV_OPT_CLOSE_ON_FREE) && fd >= 0) EVUTIL_CLOSESOCKET(fd); + + evutil_getaddrinfo_cancel_async_(bufev_p->dns_request); } static int @@ -616,6 +625,9 @@ be_socket_flush(struct bufferevent *bev, short iotype, static void be_socket_setfd(struct bufferevent *bufev, evutil_socket_t fd) { + struct bufferevent_private *bufev_p = + EVUTIL_UPCAST(bufev, struct bufferevent_private, bev); + BEV_LOCK(bufev); EVUTIL_ASSERT(bufev->be_ops == &bufferevent_ops_socket); @@ -633,6 +645,8 @@ be_socket_setfd(struct bufferevent *bufev, evutil_socket_t fd) if (fd >= 0) bufferevent_enable(bufev, bufev->enabled); + evutil_getaddrinfo_cancel_async_(bufev_p->dns_request); + BEV_UNLOCK(bufev); } From 4a53c54bf7ad66dbe879ee3c4f04daf264430fc2 Mon Sep 17 00:00:00 2001 From: Azat Khuzhin Date: Tue, 22 Mar 2016 19:29:50 +0300 Subject: [PATCH 4/7] http: get fd from be layer during connection reset Since it can be non -1, and we must close it, otherwise we will have problems. And after this patch the following tests report fd 2307 instead of 2309 fd leaks: $ valgrind --leak-check=full --show-reachable=yes --track-fds=yes --error-exitcode=1 regress --no-fork http/cancel.. ==10853== FILE DESCRIPTORS: 2307 open at exit. --- http.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/http.c b/http.c index cce68ce6..d95cf1df 100644 --- a/http.c +++ b/http.c @@ -1329,6 +1329,9 @@ evhttp_connection_reset_(struct evhttp_connection *evcon) */ bufferevent_disable_hard_(evcon->bufev, EV_READ|EV_WRITE); + if (evcon->fd == -1) + evcon->fd = bufferevent_getfd(evcon->bufev); + if (evcon->fd != -1) { /* inform interested parties about connection close */ if (evhttp_connected(evcon) && evcon->closecb != NULL) From f0e13411795203c6a160dfc8d8f0fcb1913905ff Mon Sep 17 00:00:00 2001 From: Azat Khuzhin Date: Tue, 22 Mar 2016 23:36:19 +0300 Subject: [PATCH 5/7] http: avoid leaking of fd in evhttp_connection_free() Since we do close fd there if we don't have BEV_OPT_CLOSE_ON_FREE, and evcon->fd can be incorrect (non -1), so just get it from the underlying bufferevent to fix this. And after this patch the following tests report 0 instead of 2307 fd leaks: $ valgrind --leak-check=full --show-reachable=yes --track-fds=yes --error-exitcode=1 regress --no-fork http/cancel.. ==11299== FILE DESCRIPTORS: 3 open at exit. And this is stdin/stderr/stdout. --- http.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/http.c b/http.c index d95cf1df..a12d7731 100644 --- a/http.c +++ b/http.c @@ -1244,6 +1244,9 @@ evhttp_connection_free(struct evhttp_connection *evcon) event_deferred_cb_cancel_(get_deferred_queue(evcon), &evcon->read_more_deferred_cb); + if (evcon->fd == -1) + evcon->fd = bufferevent_getfd(evcon->bufev); + if (evcon->fd != -1) { bufferevent_disable(evcon->bufev, EV_READ|EV_WRITE); shutdown(evcon->fd, EVUTIL_SHUT_WR); From 0c343afea98a6ab6c55aa3ddf5d69d41ff83ba8a Mon Sep 17 00:00:00 2001 From: Azat Khuzhin Date: Thu, 24 Mar 2016 10:27:24 +0300 Subject: [PATCH 6/7] test/http: cover NS timed out during request cancellations separatelly --- test/regress_http.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/test/regress_http.c b/test/regress_http.c index 9cf9ee5a..1906da80 100644 --- a/test/regress_http.c +++ b/test/regress_http.c @@ -1359,6 +1359,7 @@ enum http_cancel_test_type { NO_NS = 4, INACTIVE_SERVER = 8, SERVER_TIMEOUT = 16, + NS_TIMEOUT = 32, }; static void http_cancel_test(void *arg) @@ -1391,7 +1392,10 @@ http_cancel_test(void *arg) evutil_snprintf(address, sizeof(address), "127.0.0.1:%d", portnum); evdns_base_nameserver_ip_add(dns_base, address); - evdns_base_set_option(dns_base, "timeout:", "3"); + + char *timeout = (type & NS_TIMEOUT) ? "6" : "3"; + evdns_base_set_option(dns_base, "timeout:", timeout); + evdns_base_set_option(dns_base, "initial-probe-timeout:", timeout); evdns_base_set_option(dns_base, "attempts:", "1"); } @@ -4523,6 +4527,9 @@ struct testcase_t http_testcases[] = { HTTP_N(cancel_by_host_server_timeout, cancel, BY_HOST | INACTIVE_SERVER | SERVER_TIMEOUT), HTTP_N(cancel_server_timeout, cancel, INACTIVE_SERVER | SERVER_TIMEOUT), HTTP_N(cancel_by_host_no_ns_server_timeout, cancel, BY_HOST | NO_NS | INACTIVE_SERVER | SERVER_TIMEOUT), + HTTP_N(cancel_by_host_ns_timeout, cancel, BY_HOST | NO_NS | NS_TIMEOUT), + HTTP_N(cancel_by_host_ns_timeout_inactive_server, cancel, BY_HOST | NO_NS | NS_TIMEOUT | INACTIVE_SERVER), + HTTP_N(cancel_by_host_ns_timeout_server_timeout, cancel, BY_HOST | NO_NS | NS_TIMEOUT | INACTIVE_SERVER | SERVER_TIMEOUT), HTTP(virtual_host), HTTP(post), From 7a4b472925df0a3b96492b8e6347177b1c60e8ad Mon Sep 17 00:00:00 2001 From: Azat Khuzhin Date: Thu, 24 Mar 2016 13:38:05 +0300 Subject: [PATCH 7/7] http: set fd to -1 unconditioally, to avoid leaking of DNS requests Otherwise: http/cancel_by_host_ns_timeout_inactive_server: [msg] Nameserver 127.0.0.1:37035 has failed: request timed out. [msg] All nameservers have failed OK 1 tests ok. (0 skipped) ==26211== ==26211== FILE DESCRIPTORS: 3 open at exit. ==26211== Open file descriptor 2: /dev/pts/47 ==26211== ==26211== ==26211== Open file descriptor 1: /dev/pts/47 ==26211== ==26211== ==26211== Open file descriptor 0: /dev/pts/47 ==26211== ==26211== ==26211== ==26211== HEAP SUMMARY: ==26211== in use at exit: 1,112 bytes in 5 blocks ==26211== total heap usage: 149 allocs, 144 frees, 18,826 bytes allocated ==26211== ==26211== 40 bytes in 1 blocks are indirectly lost in loss record 1 of 5 ==26211== at 0x4C2BBD5: calloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) ==26211== by 0x4AAEB2: event_mm_calloc_ (event.c:3459) ==26211== by 0x498F5B: evbuffer_add_cb (buffer.c:3309) ==26211== by 0x4A0EF5: bufferevent_socket_new (bufferevent_sock.c:366) ==26211== by 0x4BFADF: evhttp_connection_base_bufferevent_new (http.c:2375) ==26211== by 0x4BFC8F: evhttp_connection_base_new (http.c:2427) ==26211== by 0x460DAA: http_cancel_test (regress_http.c:1417) ==26211== by 0x490A78: testcase_run_bare_ (tinytest.c:105) ==26211== by 0x490D5A: testcase_run_one (tinytest.c:252) ==26211== by 0x491699: tinytest_main (tinytest.c:434) ==26211== by 0x47E0E0: main (regress_main.c:461) ==26211== ==26211== 136 bytes in 1 blocks are indirectly lost in loss record 2 of 5 ==26211== at 0x4C2BBD5: calloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) ==26211== by 0x4AAEB2: event_mm_calloc_ (event.c:3459) ==26211== by 0x491FF0: evbuffer_new (buffer.c:365) ==26211== by 0x49A1BE: bufferevent_init_common_ (bufferevent.c:300) ==26211== by 0x4A0E44: bufferevent_socket_new (bufferevent_sock.c:353) ==26211== by 0x4BFADF: evhttp_connection_base_bufferevent_new (http.c:2375) ==26211== by 0x4BFC8F: evhttp_connection_base_new (http.c:2427) ==26211== by 0x460DAA: http_cancel_test (regress_http.c:1417) ==26211== by 0x490A78: testcase_run_bare_ (tinytest.c:105) ==26211== by 0x490D5A: testcase_run_one (tinytest.c:252) ==26211== by 0x491699: tinytest_main (tinytest.c:434) ==26211== by 0x47E0E0: main (regress_main.c:461) ==26211== ==26211== 136 bytes in 1 blocks are indirectly lost in loss record 3 of 5 ==26211== at 0x4C2BBD5: calloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) ==26211== by 0x4AAEB2: event_mm_calloc_ (event.c:3459) ==26211== by 0x491FF0: evbuffer_new (buffer.c:365) ==26211== by 0x49A1FB: bufferevent_init_common_ (bufferevent.c:305) ==26211== by 0x4A0E44: bufferevent_socket_new (bufferevent_sock.c:353) ==26211== by 0x4BFADF: evhttp_connection_base_bufferevent_new (http.c:2375) ==26211== by 0x4BFC8F: evhttp_connection_base_new (http.c:2427) ==26211== by 0x460DAA: http_cancel_test (regress_http.c:1417) ==26211== by 0x490A78: testcase_run_bare_ (tinytest.c:105) ==26211== by 0x490D5A: testcase_run_one (tinytest.c:252) ==26211== by 0x491699: tinytest_main (tinytest.c:434) ==26211== by 0x47E0E0: main (regress_main.c:461) ==26211== ==26211== 536 bytes in 1 blocks are indirectly lost in loss record 4 of 5 ==26211== at 0x4C2BBD5: calloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) ==26211== by 0x4AAEB2: event_mm_calloc_ (event.c:3459) ==26211== by 0x4A0E15: bufferevent_socket_new (bufferevent_sock.c:350) ==26211== by 0x4BFADF: evhttp_connection_base_bufferevent_new (http.c:2375) ==26211== by 0x4BFC8F: evhttp_connection_base_new (http.c:2427) ==26211== by 0x460DAA: http_cancel_test (regress_http.c:1417) ==26211== by 0x490A78: testcase_run_bare_ (tinytest.c:105) ==26211== by 0x490D5A: testcase_run_one (tinytest.c:252) ==26211== by 0x491699: tinytest_main (tinytest.c:434) ==26211== by 0x47E0E0: main (regress_main.c:461) ==26211== ==26211== 1,112 (264 direct, 848 indirect) bytes in 1 blocks are definitely lost in loss record 5 of 5 ==26211== at 0x4C2BBD5: calloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) ==26211== by 0x4AAEB2: event_mm_calloc_ (event.c:3459) ==26211== by 0x4D0564: evdns_getaddrinfo (evdns.c:4685) ==26211== by 0x4B13BA: evutil_getaddrinfo_async_ (evutil.c:1575) ==26211== by 0x4A139E: bufferevent_socket_connect_hostname (bufferevent_sock.c:524) ==26211== by 0x4C02DB: evhttp_connection_connect_ (http.c:2588) ==26211== by 0x4C04DD: evhttp_make_request (http.c:2643) ==26211== by 0x4615FF: http_cancel_test (regress_http.c:1504) ==26211== by 0x490A78: testcase_run_bare_ (tinytest.c:105) ==26211== by 0x490D5A: testcase_run_one (tinytest.c:252) ==26211== by 0x491699: tinytest_main (tinytest.c:434) ==26211== by 0x47E0E0: main (regress_main.c:461) ==26211== ==26211== LEAK SUMMARY: ==26211== definitely lost: 264 bytes in 1 blocks ==26211== indirectly lost: 848 bytes in 4 blocks ==26211== possibly lost: 0 bytes in 0 blocks ==26211== still reachable: 0 bytes in 0 blocks ==26211== suppressed: 0 bytes in 0 blocks --- http.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/http.c b/http.c index a12d7731..ac146075 100644 --- a/http.c +++ b/http.c @@ -1342,9 +1342,9 @@ evhttp_connection_reset_(struct evhttp_connection *evcon) shutdown(evcon->fd, EVUTIL_SHUT_WR); evutil_closesocket(evcon->fd); - bufferevent_setfd(evcon->bufev, -1); evcon->fd = -1; } + bufferevent_setfd(evcon->bufev, -1); /* we need to clean up any buffered data */ tmp = bufferevent_get_output(evcon->bufev);