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

Implement new I/O-safe traits on types #1606

Closed
wants to merge 5 commits into from

Conversation

notgull
Copy link

@notgull notgull commented Aug 11, 2022

This PR adds a new feature, io_safety, which implements AsFd/AsSocket/From<OwnedFd>/From<OwnedSocket>/Into<OwnedFd>/Into<OwnedSocket> on the types within mio. Enabling this feature requires Rust 1.63 or higher.

See also: #1588

Copy link
Collaborator

@asomers asomers left a comment

Choose a reason for hiding this comment

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

This LGT my untrained eye. I haven't tried out I/O safety yet, so I may well be missing something.

@notgull
Copy link
Author

notgull commented Aug 11, 2022

Just a heads up: clippy seems to be failing due to the addition of a lint unrelated to this PR

@Darksonn
Copy link
Contributor

Using features for this kind of stuff is generally an anti-pattern. We can never remove a feature once we add it (remove features is a breaking change), so even if we bumped the MSRV in the future, this feature would need to stay around forever.

The way this situation has been handled in other Tokio crates in the past is to simply wait until the MSRV is bumped far enough to support it without conditional compilation.

@notgull
Copy link
Author

notgull commented Aug 11, 2022

Using features for this kind of stuff is generally an anti-pattern. We can never remove a feature once we add it (remove features is a breaking change), so even if we bumped the MSRV in the future, this feature would need to stay around forever.

We could just make it a deprecated feature that does nothing once the MSRV has reached 1.63, but if you don't want to do it that way I'd understand. We could just hold off on it until the MSRV bump.

@Noah-Kennedy
Copy link

I think that is probably our best course of action.

@sunfishcode
Copy link
Contributor

Another way to handle this would be to add a build.rs that detects whether the feature is supported. I know that has tradeoffs too, but would that be acceptable here?

@Thomasdezeeuw
Copy link
Collaborator

I'm afraid this is not going into v0.8, but it's possible for v0.9 as we need to bump the MSRV for #1527 anyway. I see you have the same pr for socket2 (rust-lang/socket2#324), so let's experiment with that first since that is being bumped (to v0.5) sooner.

@notgull
Copy link
Author

notgull commented Sep 10, 2023

Closing this, because it needs a rework when the time is right.

@notgull notgull closed this Sep 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants