Add return code to _destroy methods. Prevent destroy in callbacks for the context firing them

src/context.c

@@ -532,20 +532,19 @@ getdns_context_create(struct getdns_context ** context, int set_from_os)
  * Call this to dispose of resources associated with a context once you
  * are done with it.
  */
-void
+getdns_return_t
 getdns_context_destroy(struct getdns_context *context)
 {
     if (context == NULL) {
-        return;
+        return GETDNS_RETURN_INVALID_PARAMETER;
     }
-    // If being destroyed during getdns callback, just flag it
+    // If being destroyed during getdns callback,
-    // and destroy.  See getdns_context_process_async
+    // return an error
     if (context->processing > 0) {
-        context->processing++;
+        return GETDNS_RETURN_INVALID_PARAMETER;
-        return;
     }
     if (context->destroying) {
-        return;
+        return GETDNS_RETURN_BAD_CONTEXT;
     }
     context->destroying = 1;
     cancel_outstanding_requests(context, 1);
@@ -583,7 +582,7 @@ getdns_context_destroy(struct getdns_context *context)
         GETDNS_FREE(context->my_mf, context->timeouts_by_time);
 
     GETDNS_FREE(context->my_mf, context);
-    return;
+    return GETDNS_RETURN_GOOD;
 }               /* getdns_context_destroy */
 
 /*
@@ -1216,29 +1215,12 @@ getdns_cancel_callback(struct getdns_context *context,
     getdns_transaction_t transaction_id)
 {
     RETURN_IF_NULL(context, GETDNS_RETURN_INVALID_PARAMETER);
-
-    if (context->processing) {
-		/* When called from within a callback, do not execute pending
-		 * context destroys.
-		 * The (other) callback handler will handle it.
-		 * 
-		 * ( because callbacks occur in getdns_context_cancel_request,
-		 *   and they may destroy the context )
-		 */
-	    return getdns_context_cancel_request(context, transaction_id, 1);
-    }
-
     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 */
@@ -1454,18 +1436,11 @@ getdns_context_request_timed_out(struct getdns_dns_req
     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;
 }
 
@@ -1566,18 +1541,10 @@ getdns_return_t getdns_context_process_async(struct getdns_context* context) {
     if (ub_poll(context->unbound_ctx)) {
         if (ub_process(context->unbound_ctx) != 0) {
             /* need an async return code? */
+            context->processing = 0;
             return GETDNS_RETURN_GENERIC_ERROR;
         }
     }
-    if (context->processing > 1) {
-        // destroyed during callbacks
-        // clear flag so destroy continues
-        context->processing = 0;
-        getdns_context_destroy(context);
-        // return bad context now that the context
-        // is destroyed
-        return GETDNS_RETURN_BAD_CONTEXT;
-    }
     // reset the processing flag
     context->processing = 0;
     if (context->extension != NULL) {
@@ -1657,10 +1624,7 @@ getdns_extension_detach_eventloop(struct getdns_context* context)
 		 * ( because callbacks occur in cancel_outstanding_requests,
 		 *   and they may destroy the context )
 		 */
-		int within_callback = context->processing;
-		if (! within_callback) {
 		context->processing = 1;
-		}
 		/* cancel all outstanding requests */
 		cancel_outstanding_requests(context, 1);
 		r = context->extension->cleanup_data(context,
@@ -1669,14 +1633,7 @@ getdns_extension_detach_eventloop(struct getdns_context* context)
 			context->extension = NULL;
 			context->extension_data = NULL;
 		}
-		if (! within_callback) {
-			if (context->processing > 1) {
 		context->processing = 0;
-				getdns_context_destroy(context);
-				return GETDNS_RETURN_BAD_CONTEXT;
-			}
-			context->processing = 0;
-		}
 	}
 	return r;
 }

src/dict.c

@@ -373,15 +373,16 @@ getdns_dict_item_free(ldns_rbnode_t * node, void *arg)
 }				/* getdns_dict_item_free */
 
 /*---------------------------------------- getdns_dict_destroy */
-void
+getdns_return_t
 getdns_dict_destroy(struct getdns_dict *dict)
 {
 	if (!dict)
-       		return;
+			return GETDNS_RETURN_INVALID_PARAMETER;
 
 	ldns_traverse_postorder(&(dict->root),
 		getdns_dict_item_free, dict);
 	GETDNS_FREE(dict->mf, dict);
+	return GETDNS_RETURN_GOOD;
 }				/* getdns_dict_destroy */
 
 /*---------------------------------------- getdns_dict_set_dict */

src/getdns/getdns.h.in

@@ -649,7 +649,7 @@ getdns_list *getdns_list_create_with_extended_memory_functions(
  * you MUST copy those instances BEFORE you destroy the list else
  * unpleasant things will happen at run-time
  */
-void getdns_list_destroy(getdns_list *this_list);
+getdns_return_t getdns_list_destroy(getdns_list *this_list);
 
 /**
  * assign the child_dict to an item in a parent list, the parent list copies
@@ -716,7 +716,7 @@ getdns_dict *getdns_dict_create_with_extended_memory_functions(
  * be aware that if you have fetched any data from the dictionary it will
  * no longer be available (you are likely to experience bad things if you try)
  */
-void getdns_dict_destroy(getdns_dict *this_dict);
+getdns_return_t getdns_dict_destroy(getdns_dict *this_dict);
 
 getdns_return_t getdns_dict_set_dict(getdns_dict *this_dict,
     const char *name, const getdns_dict *child_dict);
@@ -815,7 +815,7 @@ getdns_context_create_with_extended_memory_functions(
     void (*free) (void *userarg, void *)
 );
 
-void getdns_context_destroy(getdns_context *context);
+getdns_return_t getdns_context_destroy(getdns_context *context);
 
 getdns_return_t
 getdns_cancel_callback(getdns_context *context,

src/list.c

@@ -323,13 +323,13 @@ getdns_list_destroy_item(struct getdns_list *list, size_t index)
 }
 
 /*---------------------------------------- getdns_list_destroy */
-void
+getdns_return_t
 getdns_list_destroy(struct getdns_list *list)
 {
 	size_t i;
 
 	if (!list)
-		return;
+		return GETDNS_RETURN_INVALID_PARAMETER;
 
 	for (i = 0; i < list->numinuse; i++)
 		getdns_list_destroy_item(list, i);
@@ -337,6 +337,7 @@ getdns_list_destroy(struct getdns_list *list)
 	if (list->items)
 		GETDNS_FREE(list->mf, list->items);
 	GETDNS_FREE(list->mf, list);
+	return GETDNS_RETURN_GOOD;
 }				/* getdns_list_destroy */
 
 /*---------------------------------------- getdns_list_add_item */

src/test/check_getdns_common.c

@@ -266,7 +266,8 @@ void destroy_callbackfn(struct getdns_context *context,
     int* flag = (int*)userarg;
     *flag = 1;
     getdns_dict_destroy(response);
-    getdns_context_destroy(context);
+    ck_assert_msg(getdns_context_destroy(context) != GETDNS_RETURN_GOOD,
+        "Expected getdns_context_destroy to not succeed");
 }
 
 /*

src/test/check_getdns_common.h

@@ -85,7 +85,10 @@
       *  The CONTEXT_FREE macro is used to
       *  destroy the current context.
       */
-     #define CONTEXT_DESTROY getdns_context_destroy(context);
+     #define CONTEXT_DESTROY \
+       ASSERT_RC(getdns_context_destroy(context), \
+        GETDNS_RETURN_GOOD, \
+        "Return code from getdns_context_destroy()");
 
      /*
       *  The EVENT_BASE_CREATE macro is used to

src/test/check_getdns_context_destroy.h

@@ -198,6 +198,8 @@
 
        RUN_EVENT_LOOP;
 
+       CONTEXT_DESTROY;
+
        ck_assert_msg(flag == 1, "flag should == 1, got %d", flag);
      }
      END_TEST
@@ -223,6 +225,8 @@
        getdns_cancel_callback(context, transaction_id);
        RUN_EVENT_LOOP;
 
+       CONTEXT_DESTROY;
+
        ck_assert_msg(flag == 1, "flag should == 1, got %d", flag);
      }
      END_TEST
@@ -253,6 +257,8 @@
 
        RUN_EVENT_LOOP;
 
+       CONTEXT_DESTROY;
+
        ck_assert_msg(flag == 1, "flag should == 1, got %d", flag);
      }
      END_TEST