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

io: remove Poll from the AsyncSeek::start_seek return value #2885

Merged

Conversation

zaharidichev
Copy link
Contributor

Motivation

To remove the Poll from start_seek and rely on poll_ready instead

Fix: #1994

Signed-off-by: Zahari Dichev zaharidichev@gmail.com

Signed-off-by: Zahari Dichev <zaharidichev@gmail.com>
@Darksonn Darksonn added A-tokio Area: The main tokio crate C-enhancement Category: A PR with an enhancement or bugfix. M-io Module: tokio/io labels Sep 25, 2020
@carllerche
Copy link
Member

Thanks for getting this going 👍 Looking at this, i'm wondering it would make more sense to do something like:

trait AsyncSeek {
    fn start_seek(self: Pin<&mut Self>, position: SeekFrom) -> io::Result<()>;

   fn poll_complete(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<io::Result<()>>;
}

poll_complete would be the equivalent of Sink::poll_flush. The reason I am thinking this is it doesn't matter if any existing start_seek is in flight before starting another one... you just want to make sure all the seeks are applied.

What do you think?

@Darksonn
Copy link
Contributor

Darksonn commented Oct 4, 2020

It's a bit dangerous because you have to handle e.g. two seeks of Current(i64::MAX_VALUE), which cannot be implemented as a single seek.

@zaharidichev
Copy link
Contributor Author

@carllerche @Darksonn Just so I am sure I get this correct. Would that mean that start_seek will simply accumulate seek ops to perform and then we try to batch them and wait for them to all be applied, returning the final result in poll_complete ?

@carllerche
Copy link
Member

@Darksonn @zaharidichev How about we split the difference. start_seek returns Err if there is an in-flight seek. To ensure the in-flight is complete, you call poll_complete until Ready to know the operation completed. This way we avoid the problem of handling a queue of seek requests.

In practice, this limits the trait to 1 in-flight seek, but I think that is OK.

@Darksonn
Copy link
Contributor

Darksonn commented Oct 5, 2020

I think that's good.

loop {
match self.state {
Busy(_) => panic!("must wait for poll_ready before calling start_seek"),
Copy link
Member

Choose a reason for hiding this comment

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

I think this panic message is out of date?

Copy link
Member

@carllerche carllerche left a comment

Choose a reason for hiding this comment

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

LGTM thanks 👍 I believe there is an out of date panic message.

@Darksonn Darksonn merged commit 43bd11b into tokio-rs:master Oct 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tokio Area: The main tokio crate C-enhancement Category: A PR with an enhancement or bugfix. M-io Module: tokio/io
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider removing Poll from the AsyncSeek::start_seek return value
3 participants