Re: [PATCH 1/4] ares_cancel(): cancel requests safely

2013-04-08 Thread Daniel Stenberg

On Mon, 8 Apr 2013, Alexander Klauer wrote:


An invocation of ares_cancel() walks through the request list, calling
the callbacks of all pending requests on a channel.


Thanks a lot. Your mini-series has been merged and pushed!

--

 / daniel.haxx.se


[PATCH 1/4] ares_cancel(): cancel requests safely

2013-04-08 Thread Alexander Klauer
An invocation of ares_cancel() walks through the request list, calling
the callbacks of all pending requests on a channel. Previously, if such
a callback added a new request to the channel, the request list might
not end up empty, causing an abort by assertion failure. The present
commit ensures that precisely all requests present upon entry of
ares_cancel() are cancelled, and that adding new requests through
callbacks is safe.
---
 ares_cancel.c |   40 
 1 file changed, 20 insertions(+), 20 deletions(-)

diff --git a/ares_cancel.c b/ares_cancel.c
index e5bb050..465cc9e 100644
--- a/ares_cancel.c
+++ b/ares_cancel.c
@@ -26,33 +26,33 @@
 void ares_cancel(ares_channel channel)
 {
   struct query *query;
+  struct list_node list_head_copy;
   struct list_node* list_head;
   struct list_node* list_node;
   int i;
 
-  list_head = &(channel->all_queries);
-  for (list_node = list_head->next; list_node != list_head; )
+  if (!ares__is_list_empty(&(channel->all_queries)))
   {
-query = list_node->data;
-list_node = list_node->next;  /* since we're deleting the query */
-query->callback(query->arg, ARES_ECANCELLED, 0, NULL, 0);
-ares__free_query(query);
-  }
-#ifndef NDEBUG
-  /* Freeing the query should remove it from all the lists in which it sits,
-   * so all query lists should be empty now.
-   */
-  assert(ares__is_list_empty(&(channel->all_queries)));
-  for (i = 0; i < ARES_QID_TABLE_SIZE; i++)
-{
-  assert(ares__is_list_empty(&(channel->queries_by_qid[i])));
-}
-  for (i = 0; i < ARES_TIMEOUT_TABLE_SIZE; i++)
+/* Swap list heads, so that only those queries which were present on entry
+ * into this function are cancelled. New queries added by callbacks of
+ * queries being cancelled will not be cancelled themselves.
+ */
+list_head = &(channel->all_queries);
+list_head_copy.prev = list_head->prev;
+list_head_copy.next = list_head->next;
+list_head_copy.prev->next = &list_head_copy;
+list_head_copy.next->prev = &list_head_copy;
+list_head->prev = list_head;
+list_head->next = list_head;
+for (list_node = list_head_copy.next; list_node != &list_head_copy; )
 {
-  assert(ares__is_list_empty(&(channel->queries_by_timeout[i])));
+  query = list_node->data;
+  list_node = list_node->next;  /* since we're deleting the query */
+  query->callback(query->arg, ARES_ECANCELLED, 0, NULL, 0);
+  ares__free_query(query);
 }
-#endif
-  if (!(channel->flags & ARES_FLAG_STAYOPEN))
+  }
+  if (!(channel->flags & ARES_FLAG_STAYOPEN) && 
ares__is_list_empty(&(channel->all_queries)))
   {
 if (channel->servers)
 {
-- 
1.7.9.5