NotificationsYou must be signed in to change notification settings
Fork341
Star4k

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

joshtriplett merged 6 commits intoasync-rs:mainfromforkgull:main

May 1, 2023

Merged

Implement I/O-safe traits on types#1036

joshtriplett merged 6 commits intoasync-rs:mainfromforkgull:main

May 1, 2023

Conversation

Copy link

Contributor

notgull commentedAug 11, 2022•
edited
Loading

This PR adds a new feature,io_safety, which requires Rust 1.63 or higher. This trait implements theAsFd/AsHandle/AsSocket family of functions onasync-std's types. In addition, several types also implementFrom<OwnedFd/OwnedHandle/OwnedSocket> andInto<OwnedFd/OwnedHandle/OwnedSocket>.

Fishrock123Aug 16, 2022

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

ContributorAuthor

notgullAug 16, 2022

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 impactcfg(unix) andcfg(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 OutdatedShow resolvedHide resolved

src/fs/file.rs

		}
		}

		impl From<File> for OwnedFd {

Copy link

Member

Fishrock123Aug 16, 2022

Choose a reason for hiding this comment

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

Should this beTryFrom?

Copy link

ContributorAuthor

notgullAug 16, 2022

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 forIntoRawFd for this type. If you would think that it would be better asTryFrom I will oblige.

src/fs/file.rs OutdatedShow resolvedHide 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

Fishrock123Aug 16, 2022

Choose a reason for hiding this comment

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

Should this beTryFrom?

Copy link

ContributorAuthor

notgullAug 16, 2022

Choose a reason for hiding this comment

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

I was mirroring theIntoRawSocket implementation for this type. Should this beTryFrom? If so, I can make it.

Copy link

Contributor

yoshuawuyts commentedAug 17, 2022•
edited
Loading

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?

Copy link

Contributor

sunfishcode commentedAug 17, 2022

edit: I believe file and stdio types may actually be unsound to implement these traits on on windows?

Are you referring to theOVERLAPPED issue in Windows? It's my understanding that that's nowfixed.

sunfishcode mentioned this pull request

Aug 22, 2022

build: update async-std and disable default featuressunfishcode/io-lifetimes#42

Merged

Copy link

Contributor

sunfishcode commentedAug 23, 2022

This change looks good to me. Overall, the intention forFrom<OwnedFd> andFrom<T> for OwnedFd impls is to mirror existingFromRawFd andIntoRawFd impls. In practice, the wayFromRawFd andIntoRawFd 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 referencedAug 23, 2022

Update to io-lifetimes 1.0.bytecodealliance/cap-std#273

Merged

Upcoming semver bump, to 1.0bytecodealliance/cap-std#272

Closed

Copy link

Contributor

sunfishcode commentedAug 26, 2022

Also, it's currently the case that async-std doesn't currently callReadFile orNtReadFile itself; it currently usesstd::fs::File::read from withinspawn_blocking, so async-std wouldn't be doing anything that std itself isn't already doing.

Copy link

ContributorAuthor

notgull commentedSep 18, 2022

What is the current status of this PR?

Copy link

ContributorAuthor

notgull commentedNov 9, 2022

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

A6GibKm mentioned this pull request

Dec 25, 2022

Implement AsFd for UnixStream#1050

Open

joshtriplett reviewed

Apr 30, 2023

View reviewed changes

src/fs/file.rs OutdatedShow resolvedHide resolved

joshtriplett reviewed

Apr 30, 2023

View reviewed changes

src/fs/file.rs OutdatedShow resolvedHide resolved

Copy link

Contributor

joshtriplett commentedApr 30, 2023

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

notgull added5 commits

April 30, 2023 09:58

add I/O safe traits

7c408f1

code review

e1d66f5

add end-of-file newline

c63c433

iline docs

27f26ea

Fix minor issues

dc7bb8e

notgull force-pushed themain branch from2e7c6c8 todc7bb8eCompare

April 30, 2023 17:03

joshtriplett reviewed

May 1, 2023

View reviewed changes

src/fs/file.rs OutdatedShow resolvedHide resolved

Update src/fs/file.rs

c177103

Co-authored-by: Josh Triplett <josh@joshtriplett.org>

joshtriplett merged commit7c95bce intoasync-rs:main

May 1, 2023

polarathene mentioned this pull request

Jul 23, 2024

Release 1.13.0?#1081

Closed

Copy link

Member

Keruspe commentedAug 20, 2024

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.

Copy link

Contributor

joshtriplett commentedAug 23, 2024

@Keruspe Argh. :(

Would welcome PRs fixing this; that should happen before a new release.

Copy link

Member

Keruspe commentedAug 23, 2024

Should be fixed in master now, CI is green now

Labels

None yet

6 participants

		runs-on:${{ matrix.os }}
		strategy:
		matrix:
		os:[ubuntu-latest, windows-latest]

Movatterモバイル変換

Implement I/O-safe traits on types#1036

Implement I/O-safe traits on types#1036

Uh oh!

Conversation

notgull commentedAug 11, 2022• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

Fishrock123Aug 16, 2022

Choose a reason for hiding this comment

Uh oh!

notgullAug 16, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Fishrock123Aug 16, 2022

Choose a reason for hiding this comment

Uh oh!

notgullAug 16, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Fishrock123Aug 16, 2022

Choose a reason for hiding this comment

Uh oh!

notgullAug 16, 2022

Choose a reason for hiding this comment

Uh oh!

yoshuawuyts commentedAug 17, 2022• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

sunfishcode commentedAug 17, 2022

Uh oh!

sunfishcode commentedAug 23, 2022

Uh oh!

sunfishcode commentedAug 26, 2022

Uh oh!

notgull commentedSep 18, 2022

Uh oh!

notgull commentedNov 9, 2022

Uh oh!

Uh oh!

Uh oh!

joshtriplett commentedApr 30, 2023

Uh oh!

Uh oh!

Keruspe commentedAug 20, 2024

Uh oh!

joshtriplett commentedAug 23, 2024

Uh oh!

Keruspe commentedAug 23, 2024

Uh oh!

Uh oh!

notgull commentedAug 11, 2022•
edited
Loading

yoshuawuyts commentedAug 17, 2022•
edited
Loading