Repository navigation
EventQueue: never lose a consumer wake-up in next_event_async - #115
Open
hash-money wants to merge 1 commit into
Open
hash-money wants to merge 1 commit into
hash-money wants to merge 1 commit into
Conversation
`EventFuture::poll` used `try_lock` on the queue and on the waker slot. A
poll that found either one locked returned `Pending` without registering a
waker, and nothing polls such a future again: the only wake-ups come from
`push_event` / `event_handled` taking the slot, so every later push found
it empty and woke nobody. The sequence that reaches it is ordinary: pusher
A wakes the consumer from inside its `if let … .lock().await.take()` body
(the guard lives through the body), the consumer's poll lands while pusher
B holds the queue inside `persist_queue` (a VSS write on a phone takes
hundreds of milliseconds), both `try_lock`s fail, and from then on the
wallet keeps queueing events that `wait_next_event` never returns.
Replace the slot with a `tokio::sync::Notify`: pushes and acknowledgements
`notify_one` after releasing the queue lock, and `next_event_async`
registers its wait (`Notified::enable`) before it checks the queue, so a
change between the check and the wait is never lost. A dropped wait hands a
notification it received on to the next waiter, so racing the future
against a shutdown cannot swallow an event either.
The new test reproduces the interleaving with a store whose writes are
gated and a waker that polls the consumer synchronously, as the `block_on`
thread does; it fails on the previous code ("polled 2 time(s), never
handed the event").
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
EventFuture::pollusestry_lockon the queue mutex and on the waker slot. A poll that finds either one locked returnsPendingwithout registering a waker, and nothing polls such a future again: the only wake-ups come frompush_event/event_handledtaking the slot, so every later push finds it empty and wakes nobody.The sequence that reaches it is ordinary once two pushers overlap (the LDK handler, the rebalancer and the trusted-tier listener each run as their own task, and
finish_mpp_setuppushes from thepaypath):if let Some(waker) = self.waker.lock().await.take() { waker.wake(); }. The scrutinee'sMutexGuardtemporary lives through the body, sowake()runs with the slot locked.next_event_async(Wallet::wait_next_event, or a caller's ownblock_on), is unparked by thatwake()and polls on its own thread, so its poll can land before A's task drops the guard, while pusher B holds the queue inside its ownpersist_queue(a VSS round trip from a phone takes hundreds of milliseconds or more).try_locks fail:Poll::Pending, no waker registered. B completes, findsNone, wakes nobody. From then on the wallet keeps queueing events thatwait_next_eventnever returns.We hit this on a mainnet phone behind a VSS store. The process had been up 27 hours; for at least the last twelve the consumer sat parked in the
block_ondrivingnext_event_async(JDWP thread dump), aPaymentReceivedwas logged and its queue write completed without ever being delivered, and twoPaymentSuccessfuland aPaymentFailedfrom the same period never arrived either:Fix: replace the waker slot with a
tokio::sync::Notify. Pushes and acknowledgements callnotify_one()after releasing the queue lock, andnext_event_asyncregisters its wait (Notified::enable) before it locks and checks the queue, so a change between the check and the wait is never lost (notify_onewith no waiter stores a permit). A wait dropped after being notified passes the notification on (Notified::drop), so racing the future against a shutdown cannot swallow an event.EventFuturegoes away. This is also where this queue had diverged from ldk-node's, whoseEventFuture::polllocks astd::sync::Mutexand nevertry_locks. That shape does not transfer: ldk-node encodes under astdlock and persists outside it, while this queue holds atokio::sync::Mutexacrosspersist_queueso pushes persist in order, and a tokio mutex cannot be taken synchronously frompoll.Notifyis the primitive for that; the loop is the pattern from tokio's ownNotified::enabledocs.Test:
a_poll_that_lands_on_two_busy_pushers_is_still_wokenforces the interleaving with a store whose writes wait on a gate and a waker that polls the consumer synchronously, standing in for theblock_onthread. Onmasterit fails withpolled 2 time(s), never handed the event. It is deterministic on the current-thread runtime#[tokio::test]uses: tokio's mutex is FIFO, so A's release hands the queue to B before B runs; the test's waker polls the consumer synchronously inside A'swake(); and B cannot run until A's task yields after it.Checked:
cargo fmt --check, the clippy line fromci/check-lint.sh,RUSTDOCFLAGS=-D warnings cargo doc --no-deps,cargo test -p orange-sdk --lib,cargo check --features _test-utils --tests. The_test-utilsintegration suite needs bitcoind/electrs and was not run here.Until this lands we carry it on a fork pin.