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.

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

docs: Clarified the point about changing the file descriptor underlyi… #3430

Merged
merged 3 commits into from Jan 17, 2021
Merged

docs: Clarified the point about changing the file descriptor underlyi… #3430

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9908aa0
docs: Clarified the point about changing the file descriptor underlyi…
hgomersall Jan 15, 2021
a199b9f
Improved wording in response to feedback.
hgomersall Jan 15, 2021
d12765f
Update tokio/src/io/async_fd.rs from @Darksonn
hgomersall Jan 17, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 5 additions & 2 deletions tokio/src/io/async_fd.rs
View file
Open in desktop
Expand Up @@ -24,8 +24,11 @@ use std::{task::Context, task::Poll};
///
/// The inner object is required to implement [`AsRawFd`]. This file descriptor
/// must not change while [`AsyncFd`] owns the inner object. Changing the file
/// descriptor results in unspecified behavior in the IO driver, which may
/// include breaking notifications for other sockets/etc.
/// descriptor through the underlying [`AsRawFd::as_raw_fd`] results
/// in unspecified behavior in the IO driver, which may include breaking
/// notifications for other sockets/etc. That is not to say that simply using
/// a clone of the inner object would be problematic if that is a supported
/// thing to do.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't really like this wording. Some notes:

  • The fd is not changed through as_raw_fd.
  • I'm not sure we have to expand on the kinds of ways it may break.
  • I don't want to make promises around clones of the fd. Those can easily get you in trouble too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Isn't the point that as_raw_fs exposes the actual raw file descriptor?

If clones of File might cause UB, then we have a problem (and probably new should be unsafe).

Notwithstanding the huge number of ways in which things can break, there is a definitely a need to provide more guidance around common use cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To clarify the first point, if the raw file fd wasn't exposed, would there be a problem?

Copy link
Contributor

@Darksonn Darksonn Jan 15, 2021 •
edited

Choose a reason for hiding this comment

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

I mean, the AsyncFd access the fd through as_raw_fd, but you don't change it using as_raw_fd. And the kinds of trouble are unspecified behavior, not undefined behavior. My second point was referring to "which may include breaking notifications for other sockets/etc."

Copy link
Contributor

Choose a reason for hiding this comment

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

To clarify, the problem happens when as_raw_fd returns different values when called multiple times, but you don't make it return different values through as_raw_fd — it doesn't provide mutable access to the fd.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a statement about the implementer of AsRawFd - if that does something strange, then you have a problem? I'm not sure of the right way of phrasing "done by but not done right here" which was the intention of "through the". How about "Changing the file descriptor through an unexpected implementation of AsRawFd..."?

Perhaps you could clarify the difference between unspecified behaviour and undefined behaviour...?

Copy link
Contributor

Choose a reason for hiding this comment

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

One possible way to word it would be:

The inner type must always return the same file descriptor, even if as_raw_fd is called multiple times. Violating this requirement can cause unspecified behavior.

Unspecified behavior means it does weird stuff. Undefined behavior means unsafe code gone wrong.

Copy link
Contributor

Choose a reason for hiding this comment

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

Or maybe reverse the sentence.

The as_raw_fd method on the inner type must always return the same file descriptor when called multiple times.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like the last one. I'll do that...

///
/// Polling for readiness is done by calling the async functions [`readable`]
/// and [`writable`]. These functions complete when the associated readiness
Expand Down