Fix lost-wakeup deadlock in asynchronous_sink flush (event primitive) (#255)

[Burgch]

Fix lost-wakeup deadlock in asynchronous_sink flush (event primitive)

What this fixes

boost::log::core::flush() on an asynchronous_sink can hang forever: the sink's
feeding thread parks in the event's wait() after a requested wakeup is lost, and
the flushing thread waits indefinitely for it. Full analysis in the linked issue
(#255).

Closes #255.

This is not platform-specific: the buggy atomic_based_event is the event
implementation selected on both Linux (futex) and modern Windows 8+
(WaitOnAddress), since both provide a native atomic int32 wait/notify. The
distinct winapi_based_event (used only on pre-WaitOnAddress Windows) has the
same defect in its own set_signalled(). This PR fixes both.

The change

libs/log/src/event.cpp — set_signalled() had a fast path that, when m_state
was already non-zero, issued only a release fence and skipped the wake. Both
atomic_based_event and winapi_based_event carried it.

atomic_based_event::set_signalled():

// before
if (m_state.load(relaxed) != 0u)
    atomic_thread_fence(release);              // no notify
else if (m_state.exchange(1u, release) == 0u)
    m_state.notify_one();

// after
if (m_state.exchange(1u, acq_rel) == 0u)
    m_state.notify_one();

winapi_based_event::set_signalled() gets the same treatment, keeping its
existing notify_one() (native WaitOnAddress) / SetEvent() (kernel handle)
split:

// after
if (m_state.exchange(1u, acq_rel) == 0u)
{
    if (!m_event) m_state.notify_one();
    else          SetEvent(m_event);   // (+ existing error handling)
}

The event is paired with external predicates
(unbounded_fifo_queue::m_interruption_requested and the queue contents) that
the caller publishes immediately before calling set_signalled(). The fast path
is unsound for that usage:

• On architectures that permit StoreLoad reordering (including x86), the plain
  load of m_state can observe a stale non-zero value before the caller's
  predicate store is visible.
• When a concurrent wait() is consuming the previous signal (m_state 1 → 0),
  skipping the notify races that consumption; the waiter re-checks the predicate,
  does not yet see it, and parks with no wakeup pending.

Either way the wakeup is lost. This regressed in 1.78 (commit 28822ba,
porting the futex-based event to Boost.Atomic); the previous implementation used
an unconditional exchange.

Why the fix is correct on every architecture (and why acq_rel, not seq_cst)

All synchronization funnels through the single variable m_state, so
release/acquire on that one variable is sufficient — a seq_cst total order
across multiple variables is not needed.

• Wake-on-already-consumed relies only on coherence: this exchange(1) and
  the waiter's exchange(0) in wait() are totally ordered in m_state's
  modification order. Either the waiter's exchange precedes (this one reads 0 → we
  notify) or follows (waiter reads 1 → wait() returns). No lost wakeup, on any
  memory model.
• Predicate visibility: a caller publishes its predicate with a release store
  before the call. If this exchange reads non-zero (so we skip notify), the
  waiter that consumes the pending signal reads-from this exchange (or a later RMW
  in its release sequence); its acquire exchange(0) then synchronizes-with this
  release exchange, so the caller's predicate store happens-before the waiter's
  post-wait predicate check.

acq_rel matches wait()'s exchange and is strictly stronger than the plain
release the pre-1.78 code used.

Performance

set_signalled() runs on the enqueue path (once per record), so this removes the
fast path the 1.78 change added: the already-signalled case now performs a
lock-prefixed exchange instead of a relaxed load plus release fence. The extra
cost is one atomic RMW per enqueue in the common bursty-logging case where the
event is already signalled. There is no cheaper correct alternative — a load-only
fast path is exactly what reintroduces the lost wakeup, and a seq_cst fence
before the load compiles to an equivalent locked operation on x86 — and the
notify/SetEvent itself is still gated on the 0→1 transition, so no extra wakeup
syscall is issued.

Instruction-level confirmation

Release build (gcc 14), atomic_based_event::set_signalled:

• Before: mov (%rdi),%eax; test; jne <ret> — a plain load; the
  already-signalled branch returns with no barrier and no notify.
• After: mov $0x1,%eax; xchg %eax,(%rdi); test; je <notify>; ret — the
  first instruction is the lock-implied xchg (a full barrier), and the only
  skipped notify is the correct one (this call did not transition 0 → 1).

The same was verified in the compiled Windows DLL (MSVC 14.3) for
winapi_based_event::set_signalled: the function now leads with
mov eax,1; xchg eax,[rcx] (a lock-prefixed exchange) with no plain-load fast
path, then dispatches WakeByAddressSingle (native) / SetEvent (kernel handle).

The difference between buggy and correct is exactly a plain load vs. a locked
exchange.

Test

Adds libs/log/test/run/sink_async_frontend_flush.cpp: one thread repeatedly
flushes the core while another logs to an asynchronous_sink, started together
via a barrier, for a few seconds. It mirrors the field configuration. A single
run rarely trips the race, so it is meant to be looped under stress; if the wakeup
is lost, flush() never returns and the flush thread never joins — the hang is
itself the failure signal (no watchdog needed). The Jamfile's run/*.cpp glob
wires it in automatically.

Validation

Aggressive stress loop (96 producers, 12 flushers, sub-second iterations):

• Linux / atomic_based_event (Boost 1.89.0, gcc 14): the unfixed build
  reliably stalled within a couple thousand iterations (observed at 1121 and
  1537), confirmed via gdb — feeder parked in wait() with m_state == 0 while
  m_interruption_requested == 1 and a flusher blocked on m_BlockCond. With the
  fix, the identical loop ran 10000 times with zero stalls.
• Windows 11 x64 / winapi_based_event (Boost 1.89.0, MSVC 14.3): the same
  path forced on; the unfixed build stalled at iteration 117, cdb showing the
  identical signature (feeder in winapi_based_event::wait / WaitOnAddress,
  m_state == 0, kernel handle NULL, m_interruption_requested == 1). With the
  fix, 10000 iterations with zero stalls.
• Both captures show the two livelock-ruling-out mechanisms (core's exclusive lock
  held across the flush; consume() back-pressure), confirming a genuine
  lost-wakeup rather than a queue that never drains.
• The full libs/log/test suite continues to build and pass.

Why winapi_based_event needed the same fix

winapi_based_event::set_signalled() carried the identical "already signalled →
release fence only, skip the wake" fast path, and both of its sub-paths share
the same lost-wakeup defect for the same reason (the fast path only loads
m_state, so the caller never joins m_state's release sequence and the waiter's
acquiring exchange(0) need not synchronize-with it, leaving the paired predicate
store invisible on the post-wait recheck):

• Native-wait sub-path (m_event == NULL, when boost::atomic reports a
  native wait/notify): the direct analogue of atomic_based_event. This is the
  sub-path reproduced live on Windows.
• Kernel-event sub-path (m_event != NULL): the auto-reset event's sticky
  SetEvent does not save it, because the fast path skips SetEvent entirely
  — there is no persisted signal to fall back on. An interrupt's wake can be
  collapsed into a prior enqueue's SetEvent that the feeder has already consumed
  and auto-reset, stranding the feeder with the predicate set and the kernel event
  unsignalled.

The unconditional exchange(1u, acq_rel) (keeping the existing
!m_event ? notify_one() : SetEvent() split) repairs both sub-paths: even when
the exchange reads a stale non-zero and skips the wake, it still writes m_state,
inserting the caller into the release sequence so the waiter synchronizes-with it,
and the locked RMW closes the x86 StoreLoad window.

Note on platform selection: on Windows 8+ the whole winapi_based_event class is
not selected — boost::atomic<uint32> has a native WaitOnAddress, so
event.hpp selects atomic_based_event (the same class as Linux), which the
first part of this PR fixes. winapi_based_event is reached only on
pre-WaitOnAddress Windows. Lowering BOOST_USE_WINAPI_VERSION does not flip the
selection — the capability is detected from WaitOnAddress availability at build
time, not gated on the target-version macro — so the Windows repro forced the
winapi_based_event path explicitly.

Tooling and review

Claude Code (Anthropic) was used during this investigation — to help reproduce
the stall, reason about the memory model, and draft the change and this
description. I reviewed all of it thoroughly: I am a software engineer with 15+
years of C++ experience, I have independently verified the root-cause analysis,
the memory-ordering argument, the disassembly, and the reproduction on both
Linux and Windows, and I stand behind the change as my own.

License

This contribution is licensed under the Boost Software License version 1.0.

[Lastique]

I think, an ofstream to /dev/null (or NUL on Windows) would be better here. Or a special sink backend that discards the log record. We don't want to hog arbitrary amount of memory during the test.

[Lastique]

acq_rel is not needed here.

Also, the comment above is misleading. Event is a generic component, it has no relation to sinks, or the context in which it is used.

Same applies below, to winapi_based_event::set_signalled.

[Burgch]

Ah yes, I agree - release is sufficient because we only need to guarantee that writes which occurred before signalling are published and visible to the waiter after it is woken up - we don't need to read anything else after this, so no need for acquire. I also confirmed that, on x86 at least, both acq_rel and release compile to exactly the same assembly - though I believe the same would not be true on a weak memory model architecture.

[Lastique]

I've also left some comments in the issue regarding the problem. It isn't clear to me what the problem is.