tree dbd3fa4f28ace0c24da8cf1629f5563e69dc28c3
parent 9a1f57351d0d5a4ad4c28fed13848463bf0063c7
author Willy Tarreau <w@1wt.eu> 1560153064 +0200
committer Willy Tarreau <w@1wt.eu> 1560156832 +0200
encoding latin1

BUG/MEDIUM: init/threads: prevent initialized threads from starting before others

Since commit 6ec902a ("MINOR: threads: serialize threads initialization")
we now serialize threads initialization. But doing so has emphasized another
race which is that some threads may actually start the loop before others
are done initializing.

As soon as all threads enter the first thread_release() call, their rdv
bit is cleared and they're all waiting for all others' rdv to be cleared
as well, with their harmless bit set. The first one to notice the cleared
mask will progress through thread_isolate(), take rdv again preventing
most others from noticing its short pass to zero, and this first one will
be able to run all the way through the initialization till the last call
to thread_release() which it happily crosses, being the only one with the
rdv bit, leaving the room for one or a few others to do the same. This
results in some threads entering the loop before others are done with
their initialization, which is particularly bad. PiBa-NL reported that
some regtests fail for him due to this (which was impossible to reproduce
here, but races are racy by definition). However placing some printf()
in the initialization code definitely shows this unsychronized startup.

This patch takes a different approach in three steps :
  - first, we don't start with thread_release() anymore and we don't
    set the rdv mask anymore in the main call. This was initially done
    to let all threads start toghether, which we don't want. Instead
    we just start with thread_isolate(). Since all threads are harmful
    by default, they all wait for each other's readiness before starting.

  - second, we don't release with thread_release() but with
    thread_sync_release(), meaning that we don't leave the function until
    other ones have reached the point in the function where they decide
    to leave it as well.

  - third, it makes sure we don't start the listeners using
    protocol_enable_all() before all threads have allocated their local
    FD tables or have initialized their pollers, otherwise startup could
    be racy as well. It's worth noting that it is even possible to limit
    this call to thread #0 as it only needs to be performed once.

This now guarantees that all thread init calls start only after all threads
are ready, and that no thread enters the polling loop before all others have
completed their initialization.

Please check GH issues #111 and #117 for more context.

No backport is needed, though if some new init races are reported in
1.9 (or even 1.8) which do not affect 2.0, then it may make sense to
carefully backport this small series.