use async/await and update http, tokio #138

schrieveslaach · 2020-01-02T13:40:53Z

No description provided.

dkregistry-rs needs a newer version of tokio (see camallo/dkregistry-rs#138)

dkregistry-rs and shiplift need newer version of tokio (see camallo/dkregistry-rs#138 and softprops/shiplift#191)

schrieveslaach · 2020-01-06T16:21:45Z

@lucab, @steveej, this PR adds support for async/await. Please, have a look.

Cargo.toml

dkregistry-rs and shiplift need newer version of tokio (see camallo/dkregistry-rs#138 and softprops/shiplift#191)

lucab · 2020-01-07T10:15:10Z

Cargo.toml

 sha2 = "^0.8.0"

 [dev-dependencies]
 env_logger = "0.7"
 spectral = "0.6"
+tokio = { version = "0.2", features = ["macros"] }


To the best of my knowledge, cargo will do a union of the features-set between this and the other tokio dependency.

You can either promote the macros feature to the other dependency, or avoid using macros in examples (my preference is for the latter). At that point, you can simply drop this line.

I think and tested that dev-dependencies won't be exposed to users of this crate. If I try to use following code in a rust application, the compiler tells me that it cannot find the code that is only available behind the feature macros.

#[tokio::main] async fn main() -> Result<(), Error> { // … }

| | #[tokio::main] | ^^^^ could not find `main` in `tokio`

src/errors.rs

src/lib.rs

steveej

Thanks a lot for porting this to async/await! I had started the work before the holidays and was only halfway through.

When I did, I started looking into more ergonomic ways of streaming as I found them a bit cumbersome to use. I added it one of my comments but it's worth copying here that I want to experiment with using https://github.com/tokio-rs/async-stream. I don't want to necessarily block this PR on it though, and I'm fine working on the refactor myself or review yours in a subsequent PR, whatever you prefer.

Further notes (partially redundant with the comments as well)

Please make sure to have dependency commits separate and precede the other changes
Please add lines to your PR comment for automatically closing the existing dependabot PRs for the bumped dependencies

Cargo.toml

src/lib.rs

steveej · 2020-01-07T16:14:23Z

src/v2/catalog.rs


 #[derive(Debug, Default, Deserialize, Serialize)]
 struct Catalog {
 pub repositories: Vec<String>,
 }

 impl v2::Client {
- pub fn get_catalog(&self, paginate: Option<u32>) -> StreamCatalog {
+ pub fn get_catalog<'a, 'b: 'a>(&'b self, paginate: Option<u32>) -> StreamCatalog<'a> {


When I started to take at look at porting dkregistry to async/.await, I was pointed towards https://github.com/tokio-rs/async-stream for stream handling. Do you by chance have experience with it, or are otherwise willing to compare it to what's currently in this PR?

I don't have any experience with it. Do you mind to clean this up in another PR?

That's fine by me.

After taking another look at the code and spotting the unsafe which is used within this function, I suggest the following:

Suggested change

pub fn get_catalog<'a, 'b: 'a>(&'b self, paginate: Option<u32>) -> StreamCatalog<'a> {

pub async fn get_catalog(&self, paginate: Option<u32>) -> Result<String> {

Result<String> seems a bit limited too me because as a user I would have to parse the string. I think the method should return Result<Vec<String>>. What do you think?

Result<String> matches the previous Box<dyn futures::Stream<Item = String, Error = Error>>. If we return a Vec<String> here, the caller will collect into a Vec<Vec<String>> which then needs to be flattened to Vec<String>. It would be more convenient for the consumer to collect into that directly. #140 implements my suggestion.

Okay. I wasn't aware that you can use futures for streams. But it makes totally sense.

src/v2/mod.rs

tests/mock/api_version.rs

steveej · 2020-01-07T16:23:59Z

tests/mock/catalog.rs

 // TODO(lucab): implement pagination
- assert_eq!(page2, None);
+ assert!(page2.is_none());


This change makes the panic less helpful because it hides the value which was received.

Unfortunately, I had to make this due to a limitation of error_chain:

Because the error type contains std::error::Error+Send + 'static objects, it can't implement PartialEq for easy comparisons.

The original code results in a compiler error:

| assert_eq!(page2, None); | ^^^^^^^^^^^^^^^^^^^^^^^^ | | | std::option::Option<std::result::Result<std::string::String, mock::api_version::dkregistry::errors::Error>> | std::option::Option<_>

What do you propose to improve the code?

steveej · 2020-01-07T16:24:08Z

tests/mock/catalog.rs

- let (end, _) = runtime.block_on(next.into_future()).ok().unwrap();
- assert_eq!(end, None);
+ let (end, _) = runtime.block_on(next.into_future());
+ assert!(end.is_none());


same as above

steveej · 2020-01-07T16:24:34Z

tests/mock/tags.rs

- let (end, _) = runtime.block_on(stream_rest.into_future()).ok().unwrap();
- assert_eq!(end, None);
+ let (end, _) = runtime.block_on(stream_rest.into_future());
+ assert!(end.is_none());


same as above

dkregistry-rs and shiplift need newer version of tokio (see camallo/dkregistry-rs#138 and softprops/shiplift#191)

steveej

FYI, I'm currently working on this branch to figure out a solution to my requested changes so far. Also to fix the tests which are gated by the test-net feature. Do you have any preference of either getting my changes back into this PR, or me creating a new PR with my changes squashed into your commits?

steveej · 2020-01-13T13:17:20Z

src/v2/catalog.rs

 "failed to parse url from string '{}': {}",
 ep, e
- )))));
+ )))]));
+ return unsafe { Pin::new_unchecked(b) };


I would really like to avoid the unsafe is possible, and I think it is, see: https://github.com/camallo/dkregistry-rs/pull/138/files#r365796988

schrieveslaach · 2020-01-20T14:15:55Z

@steveej, how do you want to proceed with this PR? Do you want me to squash the commits and you finished the rest in #140?

steveej · 2020-01-20T16:19:43Z

@schrieveslaach

Do you want me to squash the commits and you finished the rest in #140?

That sounds good to me. I'll rebase on top of your squashed branch, then Github will automatically detect this one as merged. I'd appreciate if you could help review #140 as well!

- Add support for async/await - Clean up dependencies - Apply cargo fmt - Bump next minor version

schrieveslaach · 2020-01-23T15:01:16Z

@steveej, I squashed the commits. I will have a look at #140 in the next couple of days.

steveej · 2020-01-24T10:46:53Z

@steveej, I squashed the commits. I will have a look at #140 in the next couple of days.

Ah, thanks for that, but could I ask you to split the commtis which touch the Cargo.* files from the code changes, please? We have tried to separate these changes in case code changes need to be reverted without changing the dependencies at one point.

steveej · 2020-01-24T12:46:56Z

I took the liberty of helping you out with splitting up the commits while preserving your authorship as best as I could. Please take a look at the commit history of #140 and let me know if that's fine for you. I suggest we either close this PR and focus on #140, but I'll leave it up to you.

schrieveslaach · 2020-01-28T08:25:26Z

@steveej, that's fine for me. 👍

schrieveslaach changed the title ~~update http requirement from 0.1 to 0.2~~ [draft] update http, tokio and use async/await Jan 3, 2020

schrieveslaach added a commit to aixigo/PREvant that referenced this pull request Jan 3, 2020

Make Infrastructure trait async (WIP)

bff4a4e

dkregistry-rs needs a newer version of tokio (see camallo/dkregistry-rs#138)

schrieveslaach added a commit to aixigo/PREvant that referenced this pull request Jan 3, 2020

Make Infrastructure trait async (WIP)

951b03e

dkregistry-rs and shiplift need newer version of tokio (see camallo/dkregistry-rs#138 and softprops/shiplift#191)

schrieveslaach force-pushed the master branch from 5290199 to f51d249 Compare January 6, 2020 16:09

schrieveslaach changed the title ~~[draft] update http, tokio and use async/await~~ use async/await and update http, tokio Jan 6, 2020

schrieveslaach force-pushed the master branch from f51d249 to cb6141b Compare January 6, 2020 16:18

schrieveslaach commented Jan 6, 2020

View reviewed changes

Cargo.toml Show resolved Hide resolved

schrieveslaach added a commit to aixigo/PREvant that referenced this pull request Jan 6, 2020

Make Infrastructure trait async (WIP)

ab9cda1

dkregistry-rs and shiplift need newer version of tokio (see camallo/dkregistry-rs#138 and softprops/shiplift#191)

lucab reviewed Jan 7, 2020

View reviewed changes

steveej suggested changes Jan 7, 2020

View reviewed changes

schrieveslaach added a commit to aixigo/PREvant that referenced this pull request Jan 13, 2020

Make Infrastructure trait async (WIP)

230ab70

dkregistry-rs and shiplift need newer version of tokio (see camallo/dkregistry-rs#138 and softprops/shiplift#191)

steveej reviewed Jan 14, 2020

View reviewed changes

steveej mentioned this pull request Jan 14, 2020

bump futures and friends and use async/await where possible #140

Merged

Update tokio and use async/await

2097f45

- Add support for async/await - Clean up dependencies - Apply cargo fmt - Bump next minor version

schrieveslaach force-pushed the master branch from d4ac836 to 2097f45 Compare January 23, 2020 14:59

schrieveslaach closed this Jan 28, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

use async/await and update http, tokio #138

use async/await and update http, tokio #138

schrieveslaach commented Jan 2, 2020

schrieveslaach commented Jan 6, 2020

lucab Jan 7, 2020

schrieveslaach Jan 10, 2020

steveej left a comment

steveej Jan 7, 2020

schrieveslaach Jan 10, 2020

steveej Jan 13, 2020

steveej Jan 13, 2020

schrieveslaach Jan 15, 2020

steveej Jan 17, 2020

schrieveslaach Jan 20, 2020

steveej Jan 7, 2020

schrieveslaach Jan 10, 2020

steveej Jan 7, 2020

steveej Jan 7, 2020

steveej left a comment

steveej Jan 13, 2020

schrieveslaach commented Jan 20, 2020

steveej commented Jan 20, 2020 •

edited

Loading

schrieveslaach commented Jan 23, 2020

steveej commented Jan 24, 2020

steveej commented Jan 24, 2020

schrieveslaach commented Jan 28, 2020

	pub fn get_catalog<'a, 'b: 'a>(&'b self, paginate: Option<u32>) -> StreamCatalog<'a> {
	pub async fn get_catalog(&self, paginate: Option<u32>) -> Result<String> {

use async/await and update http, tokio #138

use async/await and update http, tokio #138

Conversation

schrieveslaach commented Jan 2, 2020

schrieveslaach commented Jan 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

steveej left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

steveej left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schrieveslaach commented Jan 20, 2020

steveej commented Jan 20, 2020 • edited Loading

schrieveslaach commented Jan 23, 2020

steveej commented Jan 24, 2020

steveej commented Jan 24, 2020

schrieveslaach commented Jan 28, 2020

steveej commented Jan 20, 2020 •

edited

Loading