Skip to content

timeout: prevent lost wakeup in TimerStub::poll - #983

Open
cuishuang wants to merge 1 commit into
cloudflare:mainfrom
cuishuang:main
Open

cuishuang wants to merge 1 commit into
cloudflare:mainfrom
cuishuang:main

Conversation

@cuishuang

@cuishuang cuishuang commented Aug 29, 2026 •

Copy link
Copy Markdown

Summary

TimerStub::poll() could miss a timer notification if Timer::fire() ran between the fired flag check and creation of the Notified future. notify_waiters() does not retain a permit for futures created after the notification, so the timer could remain pending indefinitely.

Create the Notified future before checking the fired flag once. A fire before creation is observed via the flag. A fire after creation reaches the future even before its first poll. Return the inner future directly so fast_timeout boxes a smaller future (104 bytes versus 128 bytes for the previous revision on the local toolchain).

The regression test fires the timer before creating the Notified future and verifies the wait completes.

Validation

  • cargo test -p pingora-timeout (18 passed)
  • cargo clippy -p pingora-timeout --all-targets -- -D warnings
  • cargo fmt -p pingora-timeout -- --check

@drcaramelsyrup drcaramelsyrup added the bug Something isn't working label Sep 25, 2026

@drcaramelsyrup drcaramelsyrup left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix here. I think it looks good pending one minor unnecessary bit.

Comment thread pingora-timeout/src/timer.rs Resolved
@cuishuang

Copy link
Copy Markdown
Author

@drcaramelsyrup Thanks for pointing that out! You're right—creating the Notified future is sufficient for notify_waiters(), so I've removed both lines and squashed the change into the original commit.

@drcaramelsyrup

Copy link
Copy Markdown
Collaborator

@cuishuang Thanks again, we saw a minor detail in internal review. We can apply it as another commit on top if desired or let you revise.

Comment thread pingora-timeout/src/timer.rs Resolved
Comment thread pingora-timeout/src/timer.rs Resolved
Comment thread pingora-timeout/src/timer.rs Resolved
Signed-off-by: cuishuang <imcusg@gmail.com>
@cuishuang

Copy link
Copy Markdown
Author

@cuishuang Thanks again, we saw a minor detail in internal review. We can apply it as another commit on top if desired or let you revise.

@drcaramelsyrup Thanks for catching both of these! I updated poll() to return the inner future directly and kept only the fired flag check after creating Notified. I confirmed the future shrank from 128 to 104 bytes locally. All 18 tests and Clippy pass, and I’ve pushed the revision as a single commit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants