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

Compio IO crate #71

Merged
merged 57 commits into from
Oct 17, 2023
Merged

Compio IO crate #71

merged 57 commits into from
Oct 17, 2023

Conversation

George-Miao
Copy link
Member

@George-Miao George-Miao commented Sep 24, 2023

This PR adds compio-io and async Read/Write counterpart traits like those in tokio, futures and monoio.

  • The traits
  • Extension traits
  • Buf{Reader/Writer}
  • Util functions
  • Tests

@George-Miao
Copy link
Member Author

George-Miao commented Sep 24, 2023

Should we use RPITIT/AFIT as they're stablizing? Or stick with GAT and adopt later? @Berrysoft

@Berrysoft
Copy link
Member

Do you know how long does it take to stablize?

@George-Miao
Copy link
Member Author

George-Miao commented Sep 25, 2023

Do you know how long does it take to stablize?

I just checked, there's some concerns during FCP, but I do expect them to be resolved soon, maybe in one or two weeks.

@Berrysoft
Copy link
Member

I prefer to use them after they're stablized.

@George-Miao
Copy link
Member Author

I prefer to use them after they're stablized.

Then I'll use assoc type for now.

@Berrysoft
Copy link
Member

I changed my mind. I think we can just use AFIT, and wait for the rust team to stabilize it.

@George-Miao
Copy link
Member Author

George-Miao commented Sep 26, 2023

I changed my mind. I think we can just use AFIT, and wait for the rust team to stabilize it.

I'm thinking if we need to support dyn by adding separate trait returning boxed future instead of generated anonymous future. Dyn traits can depend on non-dyn version, makes it effectively free for trait implementor. Here's an example.

@Berrysoft
Copy link
Member

Is such trait object safe?

@George-Miao
Copy link
Member Author

George-Miao commented Sep 27, 2023

Is such trait object safe?

Yes. The main reason why AFIT/RPITIT cannot be made into dyn object is because of its anonymous assoc type, which cannot be specified. I'm using box dyn to erase that type here.

@George-Miao
Copy link
Member Author

George-Miao commented Oct 7, 2023

@Berrysoft how do you like the traits?

BTW is it possible to stop the CI for this specific PR during drafting? I'm using nightly but the CI isn't happy about that.

@George-Miao
Copy link
Member Author

George-Miao commented Oct 7, 2023

A side note: because compio is a thread-per-core runtime, we can safely disregard the lint comes with AFIT. We just need to remove it in some future version of rust.

compio-io/src/read.rs Outdated Show resolved Hide resolved
compio-io/src/read.rs Outdated Show resolved Hide resolved
compio-io/src/write.rs Outdated Show resolved Hide resolved
@Berrysoft
Copy link
Member

LGTM.

@George-Miao George-Miao marked this pull request as ready for review October 10, 2023 11:34
@George-Miao George-Miao marked this pull request as draft October 10, 2023 11:35
compio-io/src/read/buf.rs Outdated Show resolved Hide resolved
compio-io/src/write/dyn.rs Outdated Show resolved Hide resolved
@George-Miao
Copy link
Member Author

Ok I forgot one important thing. Methods in AsyncReadDyn and AsyncWriteDyn are parameterized, meaning they're not object safe. We have to give up on the dyn traits.

@Berrysoft Berrysoft mentioned this pull request Oct 11, 2023
Copy link
Member

@Berrysoft Berrysoft left a comment

Choose a reason for hiding this comment

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

I think we also need read_to_end and read_to_string.

compio-io/src/read/mod.rs Outdated Show resolved Hide resolved
compio-io/src/read/mod.rs Outdated Show resolved Hide resolved
compio-io/src/read/mod.rs Show resolved Hide resolved
compio-io/src/write/mod.rs Outdated Show resolved Hide resolved
compio-io/src/write/mod.rs Show resolved Hide resolved
compio-buf/src/io_buf.rs Outdated Show resolved Hide resolved
compio-io/src/buffer.rs Outdated Show resolved Hide resolved
compio-io/src/lib.rs Show resolved Hide resolved
compio-io/src/lib.rs Outdated Show resolved Hide resolved
compio-io/src/lib.rs Outdated Show resolved Hide resolved
compio-io/src/write/ext.rs Show resolved Hide resolved
@Berrysoft Berrysoft merged commit 8e57133 into compio-rs:master Oct 17, 2023
9 of 11 checks passed
@George-Miao George-Miao deleted the dev/io branch October 17, 2023 14:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants