-
Notifications
You must be signed in to change notification settings - Fork 343
Implement I/O-safe traits on types #1036
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn’t support MacOS?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes I wrote only impact cfg(unix) and cfg(windows), so I included a Unix platform (ubuntu-latest) and a Windows platform (windows-latest). If you would like me to also test on MacOS I can.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be TryFrom?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was mirroring the previous implementation for IntoRawFd for this type. If you would think that it would be better as TryFrom I will oblige.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be TryFrom?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was mirroring the IntoRawSocket implementation for this type. Should this be TryFrom? If so, I can make it.
Tagging in @sunfishcode who likely has thoughts on how we should approach this.
edit: I believe file and stdio types may actually be unsound to implement these traits on on windows?
edit: I believe file and stdio types may actually be unsound to implement these traits on on windows?
Are you referring to the OVERLAPPED issue in Windows? It's my understanding that that's now fixed.
This change looks good to me. Overall, the intention for From<OwnedFd> and From<T> for OwnedFd impls is to mirror existing FromRawFd and IntoRawFd impls. In practice, the way FromRawFd and IntoRawFd are used, users assume they have ownership-transfer semantics, so if there any issues with soundness with the new traits, they'd likely already present with the existing traits.
Also, it's currently the case that async-std doesn't currently call ReadFile or NtReadFile itself; it currently uses std::fs::File::read from within spawn_blocking, so async-std wouldn't be doing anything that std itself isn't already doing.
What is the current status of this PR?
@yoshuawuyts Is there anything I can do to move this PR forwards?
Commented on a couple of minor issues. Happy to merge this with those fixed.
Co-authored-by: Josh Triplett <josh@joshtriplett.org>
This is completely broken 😭
The feature is called io_safety and the cfg checks are done using io-safety as a feature name, so the code actually never got tested and doesn't even build.
@Keruspe Argh. :(
Would welcome PRs fixing this; that should happen before a new release.
Should be fixed in master now, CI is green now
Uh oh!
There was an error while loading. Please reload this page.
This PR adds a new feature,
io_safety, which requires Rust 1.63 or higher. This trait implements theAsFd/AsHandle/AsSocketfamily of functions onasync-std's types. In addition, several types also implementFrom<OwnedFd/OwnedHandle/OwnedSocket>andInto<OwnedFd/OwnedHandle/OwnedSocket>.See also: sunfishcode/io-lifetimes#38