Skip to content

Fix incorrect waker update condition #7656

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Jun 27, 2025
Merged

Fix incorrect waker update condition #7656

merged 1 commit into from
Jun 27, 2025

Conversation

jimmygchen
Copy link
Member

Issue Addressed

This bug was first found and partially fixed by @VolodymyrBg in #7317 - this PR applies the same fix everywhere else.

The old logic updated the waker when it already matched the context, and did nothing when it was stale:

if waker.will_wake(cx.waker()) {
    self.waker = Some(cx.waker().clone());
}

This is the wrong way around. We only want to update the waker if it doesn't match the current context:

if !waker.will_wake(cx.waker()) {
    self.waker = Some(cx.waker().clone());
}

I don't think we've ever noticed any issues, but it’s a subtle bug that could lead to missed wakeups.

@jimmygchen jimmygchen requested a review from jxs as a code owner June 27, 2025 05:42
@jimmygchen jimmygchen added bug Something isn't working ready-for-review The code is ready for review low-hanging-fruit Easy to resolve, get it before someone else does! labels Jun 27, 2025
@mergify Mergify
Copy link

mergify bot commented Jun 27, 2025

Some required checks have failed. Could you please take a look @jimmygchen? 🙏

@mergify mergify bot added waiting-on-author The reviewer has suggested changes and awaits thier implementation. and removed ready-for-review The code is ready for review labels Jun 27, 2025
pawanjay176
pawanjay176 approved these changes Jun 27, 2025
View reviewed changes
Copy link
Member

@pawanjay176 pawanjay176 left a comment

Choose a reason for hiding this comment

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

Nice catch!

@pawanjay176 pawanjay176 added ready-for-merge This PR is ready to merge. and removed waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels Jun 27, 2025
mergify bot added a commit that referenced this pull request Jun 27, 2025
@mergify
Merge of #7656
80a603f
@mergify mergify bot mentioned this pull request Jun 27, 2025
Closed
6 tasks
@mergify mergify bot merged commit 522e00f into sigp:unstable Jun 27, 2025
31 of 33 checks passed
@jimmygchen jimmygchen deleted the waker-fix branch June 27, 2025 21:42
michaelsproul added a commit that referenced this pull request Jun 29, 2025
@michaelsproul
Error from RPC send_response when request doesn't exist on the active…
ea3bb06 
… inbound requests (#7663)

Squashed commit of the following:

commit 0ce690c
Author: João Oliveira <[email protected]>
Date:   Fri Jun 27 22:50:11 2025 +0100

    Error from RPC `send_response`

    when request doesn't exist on the active inbound requests.
    And handle the error from the main service to check if it may be a data race or a critical bug

commit 522e00f
Author: Jimmy Chen <[email protected]>
Date:   Sat Jun 28 05:01:46 2025 +1000

    Fix incorrect `waker` update condition (#7656)

    This bug was first found and partially fixed by @VolodymyrBg in #7317 - this PR applies the same fix everywhere else.

    The old logic updated the waker when it already matched the context, and did nothing when it was stale:

    ```rust
    if waker.will_wake(cx.waker()) {
    self.waker = Some(cx.waker().clone());
    }
    ```

    This is the wrong way around. We only want to update the waker if it doesn't match the current context:

    ```rust
    if !waker.will_wake(cx.waker()) {
    self.waker = Some(cx.waker().clone());
    }
    ```

    I don't think we've ever noticed any issues, but it’s a subtle bug that could lead to missed wakeups.

commit 83cad25
Author: Jimmy Chen <[email protected]>
Date:   Sat Jun 28 04:21:17 2025 +1000

    Fix Rust 1.88 clippy errors & execution engine tests (#7657)

    Fix Rust 1.88 clippy errors.

commit 9b1f3ed
Author: Pawan Dhananjay <[email protected]>
Date:   Thu Jun 26 17:26:38 2025 -0700

    Add gossip check (#7652)

    N/A

      Add an additional gossip condition.

commit a0a6b93
Author: Daniel Knopik <[email protected]>
Date:   Wed Jun 25 08:22:24 2025 +0200

    Do not compute sync selection proofs for the sync duty at the current slot (#7551)

commit 8e3c5d1
Author: chonghe <[email protected]>
Date:   Wed Jun 25 13:33:17 2025 +0800

    Rust 1.89 compiler lint fix (#7644)

    Fix lints for Rust 1.89 beta compiler

commit 56b2d4b
Author: Eitan Seri-Levi <[email protected]>
Date:   Tue Jun 24 09:29:10 2025 +0300

    Remove instrumenting log level  (#7636)

    #7155

      Theres some additional places we set instrumenting log levels that wasn't covered in #7620

commit fd643c3
Author: Michael Sproul <[email protected]>
Date:   Mon Jun 23 23:11:46 2025 +1000

    Un-ignore EF test for v1.6.0-alpha.1 (#7632)

    Closes:

    - #7547

      Run the test that was previously ignored when we were between spec versions.

commit cef04ee
Author: chonghe <[email protected]>
Date:   Mon Jun 23 16:37:49 2025 +0800

    Implement `validator_identities` Beacon API endpoint (#7462)

    * #7442
michaelsproul added a commit that referenced this pull request Jul 1, 2025
@michaelsproul
Error from RPC send_response when request doesn't exist on the active…
20b49e4 
… inbound requests (#7663)

Squashed commit of the following:

commit 0ce690c
Author: João Oliveira <[email protected]>
Date:   Fri Jun 27 22:50:11 2025 +0100

    Error from RPC `send_response`

    when request doesn't exist on the active inbound requests.
    And handle the error from the main service to check if it may be a data race or a critical bug

commit 522e00f
Author: Jimmy Chen <[email protected]>
Date:   Sat Jun 28 05:01:46 2025 +1000

    Fix incorrect `waker` update condition (#7656)

    This bug was first found and partially fixed by @VolodymyrBg in #7317 - this PR applies the same fix everywhere else.

    The old logic updated the waker when it already matched the context, and did nothing when it was stale:

    ```rust
    if waker.will_wake(cx.waker()) {
    self.waker = Some(cx.waker().clone());
    }
    ```

    This is the wrong way around. We only want to update the waker if it doesn't match the current context:

    ```rust
    if !waker.will_wake(cx.waker()) {
    self.waker = Some(cx.waker().clone());
    }
    ```

    I don't think we've ever noticed any issues, but it’s a subtle bug that could lead to missed wakeups.

commit 83cad25
Author: Jimmy Chen <[email protected]>
Date:   Sat Jun 28 04:21:17 2025 +1000

    Fix Rust 1.88 clippy errors & execution engine tests (#7657)

    Fix Rust 1.88 clippy errors.

commit 9b1f3ed
Author: Pawan Dhananjay <[email protected]>
Date:   Thu Jun 26 17:26:38 2025 -0700

    Add gossip check (#7652)

    N/A

      Add an additional gossip condition.

commit a0a6b93
Author: Daniel Knopik <[email protected]>
Date:   Wed Jun 25 08:22:24 2025 +0200

    Do not compute sync selection proofs for the sync duty at the current slot (#7551)

commit 8e3c5d1
Author: chonghe <[email protected]>
Date:   Wed Jun 25 13:33:17 2025 +0800

    Rust 1.89 compiler lint fix (#7644)

    Fix lints for Rust 1.89 beta compiler

commit 56b2d4b
Author: Eitan Seri-Levi <[email protected]>
Date:   Tue Jun 24 09:29:10 2025 +0300

    Remove instrumenting log level  (#7636)

    #7155

      Theres some additional places we set instrumenting log levels that wasn't covered in #7620

commit fd643c3
Author: Michael Sproul <[email protected]>
Date:   Mon Jun 23 23:11:46 2025 +1000

    Un-ignore EF test for v1.6.0-alpha.1 (#7632)

    Closes:

    - #7547

      Run the test that was previously ignored when we were between spec versions.

commit cef04ee
Author: chonghe <[email protected]>
Date:   Mon Jun 23 16:37:49 2025 +0800

    Implement `validator_identities` Beacon API endpoint (#7462)

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

@pawanjay176 pawanjay176 pawanjay176 approved these changes

@jxs jxs Awaiting requested review from jxs jxs is a code owner

Assignees
No one assigned
Labels
bug Something isn't working low-hanging-fruit Easy to resolve, get it before someone else does! ready-for-merge This PR is ready to merge.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jimmygchen @pawanjay176