Skip to content
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.

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

Test for wakers being swapped #66

Open
yoshuawuyts opened this issue Nov 14, 2022 · 1 comment
Open

Test for wakers being swapped #66

yoshuawuyts opened this issue Nov 14, 2022 · 1 comment
Labels
enhancement New feature or request

Comments

@yoshuawuyts
Copy link
Owner

As @eholk pointed out in #57 (comment), we're not guaranteed to keep the same waker between calls to poll:

(It'd probably be good to have test coverage for this scenario, but that probably makes sense to add as a later PR.)

That said though; even though it's not guaranteed, a runtime will run into trouble if they call a future with a waker once, and then no longer associate that waker with the future. A future may be moved in between polls, polled again, but then do a "return fast" path in order to not do the expensive thing.

struct MyFuture {
    in_progress: bool,
}

impl Future for MyFuture {
    fn poll(&mut self, cx: &mut Context) -> Poll<()> {
        if self.in_progress {
            Poll::Pending
        } else {
            // actually do IO here
        }
    }
}

This is mostly a check to guard against error-prone runtime implementations. The actual right thing to do here would be to document that runtimes must associate a future with a waker after it's been passed once, so the complexity from this lives in the runtime - and we don't have to account for it in every future.

But we're not there right now, and we may not be able to mandate this. Meaning we should just test for this first.

@yoshuawuyts yoshuawuyts added the enhancement New feature or request label Nov 14, 2022
@eholk
Copy link
Collaborator

eholk commented Nov 17, 2022

This is mostly a check to guard against error-prone runtime implementations. The actual right thing to do here would be to document that runtimes must associate a future with a waker after it's been passed once, so the complexity from this lives in the runtime - and we don't have to account for it in every future.

I'm not sure this is actually desirable. To me the main thing a waker does is moves a task from the blocked list to the ready queue. If you move a task between worker threads, each thread will have a different blocked and ready queue, so it makes sense that the waker would change.

I guess you could get around this by having some behind-the-scenes indirection in the waker. There's also the issue of updating stale wakers, so maybe it is a runtime correctness issue after all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants