Opened 5 months ago

Closed 5 months ago

#26008 closed defect (fixed)

Make workqueue cancel code always get covered by tests.

Reported by: nickm Owned by: nickm
Priority: Medium Milestone: Tor: 0.3.4.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-ci, tor-tests-coverage, tor-tests-unit
Cc: Actual Points:
Parent ID: #25908 Points:
Reviewer: catalyst Sponsor: Sponsor3-can


After re-running the tests many times with the other #25908 children fixed, I found a remaining variance from the most usual test coverage. This case happened only once out of ~450 runs:

--- coverage-1525271181/workqueue.c.gcov.tmp	2018-05-02 15:59:24.758433136 -0400
+++ coverage-1525271827/workqueue.c.gcov.tmp	2018-05-02 15:59:24.760433142 -0400
@@ -198,14 +198,14 @@
         1:  tor_mutex_acquire(&ent->on_pool->lock);
         1:  workqueue_priority_t prio = ent->priority;
         1:  if (ent->pending) {
-        1:    TOR_TAILQ_REMOVE(&ent->on_pool->work[prio], ent, next_work);
-        1:    cancelled = 1;
-        1:    result = ent->arg;
+    #####:    TOR_TAILQ_REMOVE(&ent->on_pool->work[prio], ent, next_work);
+    #####:    cancelled = 1;
+    #####:    result = ent->arg;
         -:  }
         1:  tor_mutex_release(&ent->on_pool->lock);
         1:  if (cancelled) {
-        1:    workqueue_entry_free(ent);
+    #####:    workqueue_entry_free(ent);
         -:  }
         1:  return result;

Child Tickets

Change History (5)

comment:1 Changed 5 months ago by nickm

Milestone: Tor: 0.3.5.x-finalTor: 0.3.4.x-final
Owner: set to nickm
Sponsor: Sponsor3-can
Status: newaccepted

comment:2 Changed 5 months ago by nickm

Status: acceptedneeds_review

FWICT, this was happening because of a really improbable PRNG output in

In any case, I found a place where I was using a truly broken reservoir sampling algorithm in test_workqueue.c, and replaced it with a better one that should avoid this outcome.

See branch ticket26008, with PR at

comment:3 Changed 5 months ago by dgoulet

Reviewer: catalyst

comment:4 Changed 5 months ago by catalyst

Status: needs_reviewmerge_ready

Looks good to me!

There's some trailing whitespace in the changes file.

comment:5 Changed 5 months ago by nickm

Resolution: fixed
Status: merge_readyclosed

Thanks; merging!

There's some trailing whitespace in the changes file.

Shouldn't be a problem -- the changelog formatting scripts should solve that.

Note: See TracTickets for help on using tickets.