From 3f8e8ac098970a00cbb3f4fe97f51e8a1c9fa9b1 Mon Sep 17 00:00:00 2001 From: Neel Goyal Date: Wed, 5 Mar 2014 22:13:37 -0500 Subject: [PATCH] Possible fix for #21 and tests --- src/context.c | 42 ++++++++++++++++-- src/context.h | 4 ++ src/extension/libev.c | 4 -- src/extension/libevent.c | 4 -- src/extension/libuv.c | 4 -- src/general.c | 19 +++------ src/getdns/getdns_extra.h | 2 +- src/test/check_getdns_context_destroy.h | 57 +++++++++++++++++++++++++ 8 files changed, 107 insertions(+), 29 deletions(-) diff --git a/src/context.c b/src/context.c index 139ba0c0..5482967e 100644 --- a/src/context.c +++ b/src/context.c @@ -541,6 +541,9 @@ getdns_context_destroy(struct getdns_context *context) context->processing++; return; } + if (context->destroying) { + return; + } context->destroying = 1; cancel_outstanding_requests(context, 1); getdns_extension_detach_eventloop(context); @@ -1210,11 +1213,18 @@ getdns_cancel_callback(struct getdns_context *context, getdns_transaction_t transaction_id) { RETURN_IF_NULL(context, GETDNS_RETURN_INVALID_PARAMETER); + context->processing = 1; getdns_return_t r = getdns_context_cancel_request(context, transaction_id, 1); if (context->extension) { context->extension->request_count_changed(context, context->outbound_requests->count, context->extension_data); } + if (context->processing > 1) { + context->processing = 0; + getdns_context_destroy(context); + return GETDNS_RETURN_BAD_CONTEXT; + } + context->processing = 0; return r; } /* getdns_cancel_callback */ @@ -1417,7 +1427,32 @@ getdns_context_clear_outbound_request(getdns_dns_req * req) return GETDNS_RETURN_GOOD; } +getdns_return_t +getdns_context_request_timed_out(struct getdns_dns_req + *req) { + getdns_context* context = req->context; + getdns_transaction_t trans_id = req->trans_id; + getdns_callback_t cb = req->user_callback; + void *user_arg = req->user_pointer; + /* cancel the req - also clears it from outbound and cleans up*/ + getdns_context_cancel_request(context, trans_id, 0); + context->processing = 1; + cb(context, GETDNS_CALLBACK_TIMEOUT, NULL, user_arg, trans_id); + if (context->processing > 1) { + // destroyed. + context->processing = 0; + getdns_context_destroy(context); + return GETDNS_RETURN_BAD_CONTEXT; + } else { + context->processing = 0; + if (context->extension) { + context->extension->request_count_changed(context, + context->outbound_requests->count, context->extension_data); + } + } + return GETDNS_RETURN_GOOD; +} char * getdns_strdup(const struct mem_funcs *mfs, const char *s) @@ -1543,7 +1578,8 @@ getdns_return_t getdns_context_process_async(struct getdns_context* context) { return GETDNS_RETURN_GENERIC_ERROR; } ldns_rbnode_t* next_timeout = ldns_rbtree_first(context->timeouts_by_time); - while (next_timeout) { + getdns_return_t r = GETDNS_RETURN_GOOD; + while (next_timeout && r == GETDNS_RETURN_GOOD) { getdns_timeout_data_t* timeout_data = (getdns_timeout_data_t*) next_timeout->data; if (timeout_cmp(timeout_data, &key) > 0) { /* no more timeouts need to be fired. */ @@ -1560,10 +1596,10 @@ getdns_return_t getdns_context_process_async(struct getdns_context* context) { } /* fire the timeout */ - timeout_data->callback(timeout_data->userarg); + r = timeout_data->callback(timeout_data->userarg); } - return GETDNS_RETURN_GOOD; + return r; } typedef struct timeout_accumulator { diff --git a/src/context.h b/src/context.h index 211af949..bafe119d 100644 --- a/src/context.h +++ b/src/context.h @@ -157,6 +157,10 @@ getdns_return_t getdns_context_track_outbound_request(struct getdns_dns_req /* clear the outbound request from being tracked - does not cancel it */ getdns_return_t getdns_context_clear_outbound_request(struct getdns_dns_req *req); + +getdns_return_t getdns_context_request_timed_out(struct getdns_dns_req + *req); + /* cancel callback internal - flag to indicate if req should be freed and callback fired */ getdns_return_t getdns_context_cancel_request(struct getdns_context *context, getdns_transaction_t transaction_id, int fire_callback); diff --git a/src/extension/libev.c b/src/extension/libev.c index ba978363..7d34a1ac 100644 --- a/src/extension/libev.c +++ b/src/extension/libev.c @@ -73,10 +73,6 @@ static void getdns_libev_timeout_cb(struct ev_loop *loop, struct ev_timer* handle, int status) { getdns_timeout_data_t* timeout_data = (getdns_timeout_data_t*) handle->data; timeout_data->callback(timeout_data->userarg); - uint32_t rc = getdns_context_get_num_pending_requests(timeout_data->context, NULL); - struct getdns_libev_data* ev_data = - (struct getdns_libev_data*) getdns_context_get_extension_data(timeout_data->context); - request_count_changed(rc, ev_data); } /* getdns extension functions */ diff --git a/src/extension/libevent.c b/src/extension/libevent.c index 00b5e81e..805128f4 100644 --- a/src/extension/libevent.c +++ b/src/extension/libevent.c @@ -102,10 +102,6 @@ static void getdns_libevent_timeout_cb(evutil_socket_t fd, short what, void* userarg) { getdns_timeout_data_t* timeout_data = (getdns_timeout_data_t*) userarg; timeout_data->callback(timeout_data->userarg); - uint32_t rc = getdns_context_get_num_pending_requests(timeout_data->context, NULL); - struct event_data* ev_data = - (struct event_data*) getdns_context_get_extension_data(timeout_data->context); - request_count_changed(rc, ev_data); } /* getdns extension functions */ diff --git a/src/extension/libuv.c b/src/extension/libuv.c index ca242566..05ddcf53 100644 --- a/src/extension/libuv.c +++ b/src/extension/libuv.c @@ -75,10 +75,6 @@ static void getdns_libuv_timeout_cb(uv_timer_t* handle, int status) { getdns_timeout_data_t* timeout_data = (getdns_timeout_data_t*) handle->data; timeout_data->callback(timeout_data->userarg); - uint32_t rc = getdns_context_get_num_pending_requests(timeout_data->context, NULL); - struct getdns_libuv_data* uv_data = - (struct getdns_libuv_data*) getdns_context_get_extension_data(timeout_data->context); - request_count_changed(rc, uv_data); } static void diff --git a/src/general.c b/src/general.c index 721e91ab..d04fb57f 100644 --- a/src/general.c +++ b/src/general.c @@ -50,8 +50,8 @@ /* declarations */ static void ub_resolve_callback(void* mydata, int err, struct ub_result* result); -static void ub_resolve_timeout(void *arg); -static void ub_local_resolve_timeout(void *arg); +static getdns_return_t ub_resolve_timeout(void *arg); +static getdns_return_t ub_local_resolve_timeout(void *arg); static void handle_network_request_error(getdns_network_req * netreq, int err); static void handle_dns_request_complete(getdns_dns_req * dns_req); @@ -65,22 +65,14 @@ typedef struct netreq_cb_data } netreq_cb_data; /* cancel, cleanup and send timeout to callback */ -static void +static getdns_return_t ub_resolve_timeout(void *arg) { getdns_dns_req *dns_req = (getdns_dns_req *) arg; - struct getdns_context *context = dns_req->context; - getdns_transaction_t trans_id = dns_req->trans_id; - getdns_callback_t cb = dns_req->user_callback; - void *user_arg = dns_req->user_pointer; - - /* cancel the req - also clears it from outbound and cleans up*/ - getdns_context_cancel_request(context, trans_id, 0); - - cb(context, GETDNS_CALLBACK_TIMEOUT, NULL, user_arg, trans_id); + return getdns_context_request_timed_out(dns_req); } -static void +static getdns_return_t ub_local_resolve_timeout(void *arg) { netreq_cb_data *cb_data = (netreq_cb_data *) arg; @@ -99,6 +91,7 @@ ub_local_resolve_timeout(void *arg) /* cleanup the state */ GETDNS_FREE(dnsreq->my_mf, cb_data); + return GETDNS_RETURN_GOOD; } void priv_getdns_call_user_callback(getdns_dns_req *dns_req, diff --git a/src/getdns/getdns_extra.h b/src/getdns/getdns_extra.h index fabacfce..3d6be12b 100644 --- a/src/getdns/getdns_extra.h +++ b/src/getdns/getdns_extra.h @@ -63,7 +63,7 @@ getdns_return_t getdns_context_process_async(getdns_context* context); getdns_return_t getdns_context_set_use_threads(getdns_context* context, int use_threads); /* extensions */ -typedef void (*getdns_timeout_callback) (void* userarg); +typedef getdns_return_t (*getdns_timeout_callback) (void* userarg); /* context timeout data */ typedef struct getdns_timeout_data { diff --git a/src/test/check_getdns_context_destroy.h b/src/test/check_getdns_context_destroy.h index b7c80595..7302a3d7 100644 --- a/src/test/check_getdns_context_destroy.h +++ b/src/test/check_getdns_context_destroy.h @@ -202,6 +202,61 @@ } END_TEST + START_TEST (getdns_context_destroy_8) + { + /* + * destroy called immediately following getdns_address + * expect: callback should be called before getdns_context_destroy() returns + */ + struct getdns_context *context = NULL; + void* eventloop = NULL; + getdns_transaction_t transaction_id = 0; + + int flag = 0; /* Initialize flag */ + + CONTEXT_CREATE(TRUE); + EVENT_BASE_CREATE; + + ASSERT_RC(getdns_address(context, "google.com", NULL, + &flag, &transaction_id, destroy_callbackfn), + GETDNS_RETURN_GOOD, "Return code from getdns_address()"); + getdns_cancel_callback(context, transaction_id); + RUN_EVENT_LOOP; + + ck_assert_msg(flag == 1, "flag should == 1, got %d", flag); + } + END_TEST + + START_TEST (getdns_context_destroy_9) + { + /* + * destroy called immediately following getdns_address + * expect: callback should be called before getdns_context_destroy() returns + */ + struct getdns_context *context = NULL; + void* eventloop = NULL; + getdns_transaction_t transaction_id = 0; + + int flag = 0; /* Initialize flag */ + + CONTEXT_CREATE(TRUE); + // set timeout to something unreasonable + getdns_context_set_timeout(context, 1); + EVENT_BASE_CREATE; + + ASSERT_RC(getdns_address(context, "google.com", NULL, + &flag, &transaction_id, destroy_callbackfn), + GETDNS_RETURN_GOOD, "Return code from getdns_address()"); + ASSERT_RC(getdns_address(context, "getdnsapi.net", NULL, + &flag, &transaction_id, destroy_callbackfn), + GETDNS_RETURN_GOOD, "Return code from getdns_address()"); + + RUN_EVENT_LOOP; + + ck_assert_msg(flag == 1, "flag should == 1, got %d", flag); + } + END_TEST + void verify_getdns_context_destroy(struct extracted_response *ex_response) { /* @@ -234,6 +289,8 @@ tcase_add_test(tc_pos, getdns_context_destroy_5); tcase_add_test(tc_pos, getdns_context_destroy_6); tcase_add_test(tc_pos, getdns_context_destroy_7); + tcase_add_test(tc_pos, getdns_context_destroy_8); + tcase_add_test(tc_pos, getdns_context_destroy_9); suite_add_tcase(s, tc_pos); return s;