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

Implement I/O-safe traits on types #1036

Merged
merged 6 commits into from May 1, 2023
Merged

Implement I/O-safe traits on types #1036

merged 6 commits into from May 1, 2023

Conversation

notgull
Copy link
Contributor

@notgull notgull commented Aug 11, 2022 •
edited

This PR adds a new feature, io_safety, which requires Rust 1.63 or higher. This trait implements the AsFd/AsHandle/AsSocket family of functions on async-std's types. In addition, several types also implement From<OwnedFd/OwnedHandle/OwnedSocket> and Into<OwnedFd/OwnedHandle/OwnedSocket>.

See also: sunfishcode/io-lifetimes#38

@sunfishcode sunfishcode mentioned this pull request Aug 12, 2022
Open
18 tasks
Fishrock123
Fishrock123 reviewed Aug 16, 2022
View reviewed changes
.github/workflows/ci.yml
runs-on: ${{ matrix.os }}
strategy:
matrix:
os: [ubuntu-latest, windows-latest]
Copy link
Member

Choose a reason for hiding this comment

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

Doesn’t support MacOS?

Copy link
Contributor Author

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.

src/fs/file.rs Outdated Show resolved Hide resolved
src/fs/file.rs
}
}

impl From<File> for OwnedFd {
Copy link
Member

Choose a reason for hiding this comment

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

Should this be TryFrom?

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 was mirroring the previous implementation for IntoRawFd for this type. If you would think that it would be better as TryFrom I will oblige.

src/fs/file.rs Outdated Show resolved Hide resolved
src/net/tcp/listener.rs

impl From<TcpListener> for OwnedSocket {
fn from(listener: TcpListener) -> OwnedSocket {
listener.watcher.into_inner().unwrap().into()
Copy link
Member

Choose a reason for hiding this comment

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

Should this be TryFrom?

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 was mirroring the IntoRawSocket implementation for this type. Should this be TryFrom? If so, I can make it.

@yoshuawuyts
Copy link
Contributor

yoshuawuyts commented Aug 17, 2022 •
edited

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?

@sunfishcode
Copy link
Contributor

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.

@sunfishcode sunfishcode mentioned this pull request Aug 22, 2022
Merged
@sunfishcode
Copy link
Contributor

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.

This was referenced Aug 23, 2022
Merged
Closed
@sunfishcode
Copy link
Contributor

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.

@notgull
Copy link
Contributor Author

notgull commented Sep 18, 2022

What is the current status of this PR?

@notgull
Copy link
Contributor Author

notgull commented Nov 9, 2022

@yoshuawuyts Is there anything I can do to move this PR forwards?

@A6GibKm A6GibKm mentioned this pull request Dec 25, 2022
Open
joshtriplett
joshtriplett reviewed Apr 30, 2023
View reviewed changes
src/fs/file.rs Outdated Show resolved Hide resolved
joshtriplett
joshtriplett reviewed Apr 30, 2023
View reviewed changes
src/fs/file.rs Outdated Show resolved Hide resolved
@joshtriplett
Copy link
Contributor

Commented on a couple of minor issues. Happy to merge this with those fixed.

joshtriplett
joshtriplett reviewed May 1, 2023
View reviewed changes
src/fs/file.rs Outdated Show resolved Hide resolved
@notgull @joshtriplett
Update src/fs/file.rs
c177103 
Co-authored-by: Josh Triplett <josh@joshtriplett.org>
@joshtriplett joshtriplett merged commit 7c95bce into async-rs:main May 1, 2023
5 of 21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joshtriplett joshtriplett joshtriplett left review comments

@Fishrock123 Fishrock123 Fishrock123 left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@notgull @yoshuawuyts @sunfishcode @joshtriplett @Fishrock123