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

Documentation of AsyncFdReady*Guard::clear_ready is misleading #6302

Closed
oxalica opened this issue Jan 23, 2024 · 1 comment · Fixed by #6304
Closed

Documentation of AsyncFdReady*Guard::clear_ready is misleading #6302

oxalica opened this issue Jan 23, 2024 · 1 comment · Fixed by #6304
Labels
A-tokio Area: The main tokio crate C-bug Category: This is a bug. M-net Module: tokio/net T-docs Topic: documentation

Comments

@oxalica
Copy link

oxalica commented Jan 23, 2024

Version

tokio 1.35.1

Platform
N/A, documentation only

Description

The documentation of AsyncFdReadyGuard::clear_ready says:

Indicates to tokio that the file descriptor is no longer ready. All internal readiness flags will be cleared, and tokio will wait for the next edge-triggered readiness notification from the OS.

To me, this means that it clear the state and wait for the next notification after the time calling this method. But this implies that the it's incorrect to read to EAGAIN and then clear_ready (as said in docs of clear_ready_matching or try_io implementation), because there could be an edge-notification between the last read and clear_ready, and it would be cleared and lost. Thus clear_ready should be called before calling read to catch all potential notifications.

But after a quick glance to the code, clear_ready seems to only clear readiness flags before the triggered event.

if TICK.unpack(current) as u8 != t {
// Trying to clear readiness with an old event!
return;
}

That's to clear readiness before the readiness().await returns, but not notifications happen between readiness() and the next clear_ready(). So the read then clear_ready pattern will not lose any notification.

It's better to address this timing issue in clear_ready and maybe also AsyncFd to be compatible with existing examples and implementations.

@oxalica oxalica added A-tokio Area: The main tokio crate C-bug Category: This is a bug. labels Jan 23, 2024
@Darksonn Darksonn added T-docs Topic: documentation M-net Module: tokio/net labels Jan 24, 2024
@Darksonn
Copy link
Contributor

Yes, we have logic to avoid that race condition. I agree that it would be reasonable to point this out in the documetation.

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-bug Category: This is a bug. M-net Module: tokio/net T-docs Topic: documentation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants