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

Implement async versions of VarIntReader and VarIntWriter #4

Closed
devashishdxt opened this issue Nov 13, 2019 · 13 comments
Closed

Implement async versions of VarIntReader and VarIntWriter #4

devashishdxt opened this issue Nov 13, 2019 · 13 comments
Assignees

Comments

@devashishdxt
Copy link
Contributor

Using futures::io traits: AsyncRead and AsyncWrite.

@dermesser dermesser self-assigned this Feb 20, 2020
dermesser added a commit that referenced this issue Feb 21, 2020
@dermesser
Copy link
Owner

Note that I ccurrently find it hard to implement this in a straight-forward way as a super-trait of AsyncRead. I'm using the async-trait crate to help here, but there is some (hidden to me) roadblock stopping rustc from accepting my implementation in an example I wrote (the code itself compiles):

async fn read_and_verify() -> io::Result<()> {
    let mut f = tokio::fs::File::open("/tmp/varintbytes").await?;
    let i1: u32 = f.read_varint_async().await?;
    let i2: u32 = f.read_varint_async().await?;
    let i3: u32 = f.read_varint_async().await?;
    assert!(f.read_varint_async().await.is_err());

    Ok(())
}
error[E0599]: no method named `read_varint_async` found for type `tokio::fs::file::File` in the current scope
  --> src/main.rs:22:15
   |
22 |     assert!(f.read_varint_async().await.is_err());
   |               ^^^^^^^^^^^^^^^^^ method not found in `tokio::fs::file::File`
   |
   = note: the method `read_varint_async` exists but the following trait bounds were not satisfied:
           `tokio::fs::file::File : integer_encoding::reader::VarIntAsyncReader`

despite File implementing AsyncRead + Unpin + Send as required.

If I don't manage to understand it, maybe we will start by using a bare function returning a future, which will not be quite as elegant, but do the same.

@devashishdxt
Copy link
Contributor Author

@dermesser Can you push your code changes to a branch? I can have a look and try to fix the issue.

@dermesser
Copy link
Owner

@devashishdxt the example is now in branch async.

@devashishdxt
Copy link
Contributor Author

@dermesser Thanks. I’ll take a look tomorrow.

dermesser added a commit that referenced this issue Feb 22, 2020
I was using futures::AsyncRead and wondered why tokio::fs::File didn't
implement it; tokio has its own AsyncRead...

Related to: #4
@dermesser
Copy link
Owner

the mystery has been solved; I was using traits from futures and wondered why the types from tokio with didn't implement them. The confusion stems from the equally named AsyncRead etc. traits that appear in both crates.

@devashishdxt
Copy link
Contributor Author

the mystery has been solved; I was using traits from futures and wondered why the types from tokio with didn't implement them. The confusion stems from the equally named AsyncRead etc. traits that appear in both crates.

You can try using feature based approach to support both the versions. Something similar to this.

@dermesser
Copy link
Owner

I'm still skeptic about the current chaos in async rust. I believe tokio is the way forward, not the futures crate, isn't it?

@devashishdxt
Copy link
Contributor Author

I'm still skeptic about the current chaos in async rust. I believe tokio is the way forward, not the futures crate, isn't it?

I agree with you. I also think that tokio is the way to go. But, for this crate, I think it’s good to leave that choice upto users and support both approaches.

@dermesser
Copy link
Owner

I might just use this occasion to ask - do you know what exactly is the relation between tokio and futures? Are they symbiotic? Competing? I've heard more from tokio, but otherwise my opinion is based on not much more than gut feeling.

@devashishdxt
Copy link
Contributor Author

devashishdxt commented Feb 22, 2020

I might just use this occasion to ask - do you know what exactly is the relation between tokio and futures? Are they symbiotic? Competing? I've heard more from tokio, but otherwise my opinion is based on not much more than gut feeling.

There is a lot going on in Rust’s async world. If you want to understand why tokio’s AsyncRead/AsyncWrite are different from those in futures crate, read this. You can read other interviews on same blog to know about other things also.

@dermesser
Copy link
Owner

Thank you for the link. I suppose I will try using your suggested approach with the features; it should only affect the AsyncRead/AsyncWrite (and similar) traits after all. If I understand correctly, futures themselves are compatible across the runtimes/reactors in both crates, if they use the standard future type. bleugh :)

@Jens-G
Copy link

Jens-G commented Feb 27, 2020

Hi, what could be the reason that we get lots of funny errors since a few days? Seems related to this .
https://travis-ci.org/apache/thrift/jobs/655964363#L15516

@dermesser
Copy link
Owner

This does look like the reason, although there are roughly two sub-reasons:

  • I'm not sure why the Cargo.toml manifest in lib/rs/ points to 1.0 (which is the old working version) but the Travis job compiles 1.1.1.
    • Also it not only builds the feature code but also the tests, which use a syntax not enabled in that manifest ("await is a keyword in the 2018 edition"). I would have expected that the test code is not compiled at all in this case.
  • 1.1.1 has the bug/annoying feature of trying to compile the tests on asynchronous parts of the crate even if no asynchronous feature (tokio_async or futures_async) is enabled. I just published version 1.1.2 which doesn't do this anymore and tests fine without having to do anything else.

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

No branches or pull requests

3 participants