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

Switch tokio AsyncRead/Write traits to futures AsyncRead/Write tr… #57

Merged
merged 2 commits into from
Dec 9, 2022

Conversation

cmeister2
Copy link
Collaborator

…aits.

Provide option to switch between async-std and tokio for with_file_async

@cmeister2 cmeister2 requested a review from drahnr December 8, 2022 09:04
@cmeister2 cmeister2 force-pushed the cm2/multiprovider branch 2 times, most recently from bd088a7 to 748a777 Compare December 8, 2022 09:23
Cargo.toml Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
@drahnr
Copy link
Contributor

drahnr commented Dec 9, 2022

@cmeister2 could you resolve the conflicts with master? Let's merge it right after

@cmeister2
Copy link
Collaborator Author

@cmeister2 could you resolve the conflicts with master? Let's merge it right after

Should be resolved.

Copy link
Contributor

@drahnr drahnr left a comment

Choose a reason for hiding this comment

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

One feature gate is missing, other than that, LGTM!

@cmeister2
Copy link
Collaborator Author

One feature gate is missing, other than that, LGTM!

As per comment, I don't think there's a bug here.

@drahnr
Copy link
Contributor

drahnr commented Dec 9, 2022

Mea culpa, good to go

@cmeister2 cmeister2 merged commit 047325c into master Dec 9, 2022
@dralley dralley deleted the cm2/multiprovider branch December 16, 2022 16:25
@dralley
Copy link
Collaborator

dralley commented Feb 7, 2023

@cmeister2 @drahnr I recently learned that all async file I/O in Rust at the moment is actually synchronous I/O backed by a threatpool inside of the async library (tokio or async-std). And apparently there are some additional problems with the specific implementation that makes it extremely slow and not at all worth using unless you're already inside an async framework. Tokio actually recommends against using it if your workload is primarily filesystem-based.

I'm curious if there is an actual use case in mind for this feature with solid backing? At the very least I'm not sure we should have it enabled by default if it doesn't buy the user much over .spawn_blocking().

@drahnr
Copy link
Contributor

drahnr commented Feb 7, 2023

It's mostly convenience, not having to deal with the additional complexity of offloading all IO ops manually.

@cmeister2
Copy link
Collaborator Author

Generally if I'm working in async I want everything to be async, including IO operations (even if actually under the covers it's not implemented in a "pure" AIO way. I also believe Linux is making strides towards actual async IO in the kernel, so at some point in the future I could well believe that the async frameworks will just expose the underlying support, and all consumers get improvements for free.

I'm personally fine with not having async on by default though - I think it's something that we should probably have behind the existing feature flags.

@dralley
Copy link
Collaborator

dralley commented Feb 7, 2023

I also believe Linux is making strides towards actual async IO in the kernel, so at some point in the future I could well believe that the async frameworks will just expose the underlying support, and all consumers get improvements for free.

It will happen eventually but the improvements won't be free, the polling-based model used by futures / tokio / async-std is incompatible with the completion-based model of io_uring. A lot of the discussion about how to adopt those system APIs revolves around how to design a new set of traits and rewrite large portions of tokio to take advantage of it. Shims might be possible but it would come at the expense of performance.

I guess my main question is whether a user has ever articulated a compelling need for it yet. Or if we know of any that are actually using it this way.

@drahnr
Copy link
Contributor

drahnr commented Feb 7, 2023

It's already there, the user is not forced to use it. I'd prefer to leave a comment on the relevant functions, after all, we will see where things settle down regarding Io in executors. Out of our scope.

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.

None yet

3 participants