199. [bug] isc_heap_delete() sometimes violated the heap

invariant, causing timer events not to be posted
			when due.

Specifically, isc_heap_delete() moved the last element of the heap to
the vacated position and then attempted to restore the heap invariant
by calling sink_down().  This works when the last element has a lower
priority than the one being deleted, and this is often the case
because the last element tends to have a low priority.  However, it is
not guaranteed to be the lowest.  When deleting an item of a lower
priority than the last item, restoring the invariant requires a call
to float_up(), not sink_down().

isc_heap_delete_now calls either float_up() or sink_down() as needed,
and INSISTs have been added to verify that the heap invariant indeed
holds after the calls.

CHANGES

@@ -1,3 +1,7 @@
+ 199.	[bug]		isc_heap_delete() sometimes violated the heap
+			invariant, causing timer events not to be posted
+			when due.
+
  198.	[func]		Dispatch managers hold memory pools which
 			any managed dispatcher may use.  This allows
 			us to avoid dipping into the memory context for

lib/isc/heap.c

@@ -46,6 +46,15 @@
 #define VALID_HEAP(h)			((h) != NULL && \
 					 (h)->magic == HEAP_MAGIC)
 
+/*
+ * When the heap is in a consistent state, the following invariant
+ * holds true: for every element i > 1, heap_parent(i) has a higher
+ * priority than i.
+ */
+#define HEAPCONDITION(i) ((i) == 1 || \
+			  heap->compare(heap->array[heap_parent(i)], \
+					heap->array[(i)]))
+
 struct isc_heap {
 	unsigned int			magic;
 	isc_mem_t *			mctx;
@@ -130,9 +139,9 @@ static void
 float_up(isc_heap_t *heap, unsigned int i, void *elt) {
 	unsigned int p;
 
-	for ( p = heap_parent(i); 
+	for (p = heap_parent(i); 
-	      i > 1 && heap->compare(elt, heap->array[p]);
+	     i > 1 && heap->compare(elt, heap->array[p]);
-	      i = p, p = heap_parent(i) ) {
+	     i = p, p = heap_parent(i)) {
 		heap->array[i] = heap->array[p];
 		if (heap->index != NULL)
 			(heap->index)(heap->array[i], i);
@@ -140,19 +149,20 @@ float_up(isc_heap_t *heap, unsigned int i, void *elt) {
 	heap->array[i] = elt;
 	if (heap->index != NULL)
 		(heap->index)(heap->array[i], i);
+
+	INSIST(HEAPCONDITION(i));
 }
 
 static void
 sink_down(isc_heap_t *heap, unsigned int i, void *elt) {
 	unsigned int j, size, half_size;
-
 	size = heap->last;
 	half_size = size / 2;
 	while (i <= half_size) {
-		/* find smallest of the (at most) two children */
+		/* Find the smallest of the (at most) two children. */
 		j = heap_left(i);
 		if (j < size && heap->compare(heap->array[j+1],
-						     heap->array[j]))
+					      heap->array[j]))
 			j++;
 		if (heap->compare(elt, heap->array[j]))
 			break;
@@ -164,6 +174,8 @@ sink_down(isc_heap_t *heap, unsigned int i, void *elt) {
 	heap->array[i] = elt;
 	if (heap->index != NULL)
 		(heap->index)(heap->array[i], i);
+
+	INSIST(HEAPCONDITION(i));
 }
 
 isc_result_t
@@ -184,13 +196,22 @@ isc_heap_insert(isc_heap_t *heap, void *elt) {
 void
 isc_heap_delete(isc_heap_t *heap, unsigned int i) {
 	void *elt;
+	isc_boolean_t less;
 
 	REQUIRE(VALID_HEAP(heap));
 	REQUIRE(i >= 1 && i <= heap->last);
 
-	elt = heap->array[heap->last];
+	if (i == heap->last) {
-	if (--heap->last > 0)
+		heap->last--;
-		sink_down(heap, i, elt);
+	} else {
+		elt = heap->array[heap->last--];
+		less = heap->compare(elt, heap->array[i]);
+		heap->array[i] = elt;
+		if (less)
+			float_up(heap, i, heap->array[i]);
+		else
+			sink_down(heap, i, heap->array[i]);
+	}
 }
 
 void