#1 Lock not held in Distributor __standard_run()
From Peter Svensson
We have found a locking error in the new
icd_distributor__standard_run(). If the result of
link_fn() is ICD_ENOTFOUND the lock is not held when
pthread_cond_wait() is entered.
There is a conceptual problem with the current locking
system as well. Is handles the case where the link fn
will or ought to succeed as soon as there is both at
least one agent and one customer. However, this is not
enough for the ICD_ENOTFOUND support for distributors.
In that case the test
"icd_distributor__customers_pending(dist) && icd...."
and the subsequent pthread_cond_wait() are performed
without releasing the lock in between.
The lock that protects the wakeup condition is dropped
before the link_fn, so a signaling of the condition can
creep in between. Since the condition variable is
without memory it will be lost. There are several
solutions to this that springs to my mind:
1) Make the condition variable have a memory by also
having a variable that is protected by the lock and set
whenever the condition variable is signaled. Test it
under the lock before doing the pthread_cond_wait().
See below for a problem.
2) Hold the lock over the call to link_fn(). This
requires careful analysis that no path through any
distributor leads to a function where the condition is
signaled since that requires that the lock is taken.
See below.
3) Allow the race to exist, it is quite small and if
the distributor is busy something will happen soon to
trigger the condition. Add a timeout to the
pthread_cond_wait() for 20s or so just for safety.
We have chosen 3) as the quick fix.
Both 1) and 2) suffer from a common problem. Some
distributors (though not our icd_mod_matchagent)
directly or indirectly signal the condition variable
through e.g. icd_distributor__pushback_agent. This
could lead to a loop (for case 1) or a hang (for case
2). The loop would only occur if the distributor uses
ICD_ENOTFOUND to ask to be put to sleep which only our
module does as far as I know.
Another problem we face (that made us choose option 3
as the quick fix) is that some operations may cause a
previously undistributable distributor queue to be
runnable. Most of these are unlikely such as an agent
being set to the DISTRIBUTING state by another
distributor and then returned to READY. This problem is
unsolvable in general due to veto-able events in e.g.
icd_member__distribute() and the arbitrary matching
functions in icd_mod_matchagent.
I suggest that you revert the standard_run function to
the old, safe version and that icd_mod_matchagent
continues to use an internal run function until all
this is thought through.
