Update to futures 0.3.0 and async await syntax #71

marcelbuesing · 2019-10-21T07:27:05Z

Since async/await is about to be stabilized (7. Nov as far as I know) I think it would be great to have a branch until then that prepares the crate for that.

Update dependencies:
- futures = "0.1" to "0.3" (preview)
- tokio-io preview
Update to rust 2018 edition, remove some pre 2018 source code artifacts e.g. extern crates...
Added toolchain file (prior to stabilization)

Not sure why the nightly CI is failing, it works locally using nightly-2019-10-20

Cargo.toml

ramosbugs

thanks for the PR!

I'm hesitant to crate a separate branch since it may create a maintenance hassle. is it possible to offer an async/await interface alongside the existing interface, gated behind a feature flag? not sure if that compiles on older rustc versions or not.

I also don't think we're ready to require the 2018 edition for this crate. Maybe for a future 4.x release...

ramosbugs · 2019-10-22T00:47:31Z

I also don't think we're ready to require the 2018 edition for this crate. Maybe for a future 4.x release...

Thinking about this some more, as long as the crate compiles on older rustc versions with 2018 mode enabled, I'd be willing to discontinue support for 2015 with the async/await API behind a feature flag. So, there should be three interfaces in this crate:

sync
current async
async await (gated by feature flag)

marcelbuesing · 2019-10-22T14:52:26Z

Ok I just added it behind a feature flag. Good thing is I assume this will also be necessary for the next stable reqwest versions, as far as I can tell it does not look like reqwest will continue support of futures 0.1 in new versions (at least there is no feature flag gating). Downside some duplicate code, and feature flags also required in doc tests (this may be confusing I assume for some when looking at the docs). I am not entirely sure about 2018/2015 rustc downwards compatibility.

The reason why I used the runtime crate instead of the latest tokio alpha for the async/await lib doc test was that I did not figure out how to handle two different tokio dev-dependencies. Using the package renaming did work not in this case.

ramosbugs · 2019-10-27T06:46:39Z

Really appreciate the work you've put into this! I spent a bit of time testing compatibility, and it looks like this effectively sets the minimum Rust version to 1.36.0 (2019-07-04). In anything prior, I get:

error[E0721]: `await` is a keyword in the 2018 edition
   --> src/lib.rs:954:55
    |
954 |         let http_response = http_client(http_request).await.map_err(RequestTokenError::Request)?;
    |                                                       ^^^^^ help: you can use a raw identifier to stay compatible: `r#await`

error[E0721]: `await` is a keyword in the 2018 edition
    --> src/lib.rs:1360:55
     |
1360 |         let http_response = http_client(http_request).await.map_err(RequestTokenError::Request)?;
     |                                                       ^^^^^ help: you can use a raw identifier to stay compatible: `r#await`

(even with futures-03 disabled)

It's one thing to drop support for the 2015 edition, but 1.36 seems a bit too recent (<4 months) to require in order for people to use the 3.x release of this crate. I think it would be prudent to wait a couple of months before imposing the 1.36 requirement (possibly on a 4.x release train), unless we can figure out a way to make it work on older toolchains. I wonder if there's a way to use proc macros to generate the async/await code only when the feature is enabled, or something.

marcelbuesing · 2019-10-28T20:25:56Z

Fair point, I think it certainly makes sense to somehow make it work for users that can not use a 1.36+ version. I have seen multiple issues with older versions.

1.32.0

error: local variables in const fn are unstable
  --> .cargo/registry/src/github.com-1ecc6299db9ec823/percent-encoding-2.1.0/lib.rs:72:13

error[E0658]: use of unstable library feature 'try_from' (see issue #33417)
 --> .cargo/registry/src/github.com-1ecc6299db9ec823/miniz_oxide-0.3.5/src/deflate/core.rs:3:5

1.33.0

error[E0658]: use of unstable library feature 'try_from' (see issue #33417)
 --> .cargo/registry/src/github.com-1ecc6299db9ec823/miniz_oxide-0.3.5/src/deflate/core.rs:3:5

1.34.0

error[E0721]: `await` is a keyword in the 2018 edition
  --> src/reqwest.rs:96:10

1.35.0

error[E0721]: `await` is a keyword in the 2018 edition
  --> src/reqwest.rs:96:10

If you have a particular version in mind I'll try to make it work somehow. Maybe 1.34.0 would be a good candidate, I assume the await keyword problem is solvable.

ramosbugs · 2019-10-28T21:11:37Z

Interesting, I didn't realize the crate was already broken for versions < 1.34. If that's indeed the case, then I can't foresee this PR causing any new problems if we can make it work for 1.34.0 and newer versions.

I'll try to update the Travis-CI config so that it tests 1.34 compatibility going forward. I suspect breaking 1.33 and earlier was unintentional.

marcelbuesing · 2019-11-04T19:48:08Z

Ok I asked here because I did not come up with an idea how to fix the keyword problem on 1.34.
So apparently the fix would be to extract the async code into a gated module.

Regarding "Downstream can still use async.await even if you don't internally." When using async/await I'm not a big fan of using the compatibility layer in my code (although I think it's great that it exists!) since it makes the the code a lot more verbose, adding compat() before every await.

Anyway I'm kind of uncertain how to proceed, what do you think?

ramosbugs · 2019-11-05T05:26:13Z

So apparently the fix would be to extract the async code into a gated module.

Oh that's really interesting. If that indeed works, what if we:

add a new private async_internal module gated by cfg
within async_internal, define the traits AsyncCodeTokenRequest, AsyncRefreshTokenRequest, AsyncPasswordTokenRequest, and AsyncClientCredentialsTokenRequest, each with a request_async method that implements the async interface
impl each of these traits on their respective structs
add a pub use in the top-level module gated by cfg

Then, using the async interface just involves enabling the feature flag and importing the right trait(s). Would that work?

marcelbuesing · 2019-11-07T21:20:22Z

It seems to work. Still requires some cleanup and documentation and I'm unsure whether all the Send constraints are a good idea, but in general it works if the async trait crate is used. CI currently fails because cargo audit fails on 1.34.0. Besides that, I have not found a way to make tests compile on 1.34.0 because async_std does not build on 1.34.0 but can not be made optional, since apparently dev-dependencies can not be made optional.

ramosbugs · 2019-11-09T22:13:40Z

Thanks! I'm planning to look this over fully this weekend, probably tomorrow.

It seems to work. Still requires some cleanup and documentation and I'm unsure whether all the Send constraints are a good idea, but in general it works if the async trait crate is used. CI currently fails because cargo audit fails on 1.34.0. Besides that, I have not found a way to make tests compile on 1.34.0 because async_std does not build on 1.34.0 but can not be made optional, since apparently dev-dependencies can not be made optional.

I think we should be able to use cargo when here so that on 1.34.0 we just run cargo build but don't bother with cargo test since it's broken. Here's an example: https://github.com/ramosbugs/openidconnect-rs/blob/2c110de9b11f2b6328754774821ea8ae3acaac80/.travis.yml#L31

ramosbugs · 2019-11-10T04:02:40Z

src/reqwest.rs

+use reqwest::RedirectPolicy;
+
+#[cfg(feature = "futures-03")]
+use reqwest_futures_03::blocking;


ah I didn't realize futures-01 and futures-03 would be mutually exclusive. when building with --all-features, this causes:

error[E0252]: the name `blocking` is defined multiple times --> /Users/daramos/Documents/Projects/oauth2-rs/src/reqwest.rs:17:5 | 10 | use reqwest as blocking; | ------------------- previous import of the module `blocking` here ... 17 | use reqwest_futures_03::blocking; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `blocking` reimported here | = note: `blocking` must be defined only once in the type namespace of this module help: you can use `as` to change the binding name of the import | 17 | use reqwest_futures_03::blocking as other_blocking; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

is it possible to make them coexist?

ramosbugs · 2019-11-10T04:14:15Z

Cargo.toml


 [features]
-default = ["reqwest"]
+default = ["futures-01"]
+futures-01 = ["futures-0-1", "reqwest"]


can we make the reqwest/curl (or neither) feature flags orthogonal to futures-01/futures-03? ideally library users should be able to request features ["reqwest", "futures-03"] to indicate that they want both a reqwest client and future-03. the type of reqwest client can be inferred from the type of futures, I think

ramosbugs · 2019-11-10T05:19:16Z

src/async_internal.rs

+///
+/// Asynchronous HTTP client.
+///
+pub async fn async_http_client(request: HttpRequest) -> Result<HttpResponse, Error> {


would it be possible to move this to the reqwest module? I think we can do a similar trick and put this in a private module inside oauth2::reqwest and then conditionally pub use it.

to avoid the name conflict, maybe we should rename the old async interface to future_http_client and each of the old request_async interfaces to request_future? that should allow them to coexist, I think, and fix the --all-features issue. it should also make the library easier to use since users can specify any combination of features without running into compilation errors.

ramosbugs · 2019-11-10T05:31:02Z

src/reqwest.rs

-                        .map_err(Error::Reqwest)
-                })
-        })
+    // Following redirects opens the client up to SSRF vulnerabilities.


doesn't cargo fmt indent this?

Start removal of pre 2018 edition source code artifacts

This way reqwest and futures may also be used without each other.

ramosbugs · 2019-11-18T05:56:02Z

thanks again!

marcelbuesing · 2019-11-18T08:29:12Z

Awesome, I actually I did not think it would be possible to make it compile with all features! Looks great!

marcelbuesing force-pushed the async-await branch from b56111a to 754b943 Compare October 21, 2019 08:04

marcelbuesing changed the title ~~[WIP] Update to futures-preview 0.3.0-alpha.19 and async await syntax~~ Update to futures-preview 0.3.0-alpha.19 and async await syntax Oct 21, 2019

marcelbuesing commented Oct 21, 2019

View reviewed changes

Cargo.toml Outdated Show resolved Hide resolved

ramosbugs reviewed Oct 21, 2019

View reviewed changes

marcelbuesing force-pushed the async-await branch from 2403d22 to e5e2bf7 Compare November 7, 2019 20:27

marcelbuesing changed the title ~~Update to futures-preview 0.3.0-alpha.19 and async await syntax~~ Update to futures-preview 0.3.0 and async await syntax Nov 8, 2019

marcelbuesing changed the title ~~Update to futures-preview 0.3.0 and async await syntax~~ Update to futures 0.3.0 and async await syntax Nov 8, 2019

ramosbugs reviewed Nov 10, 2019

View reviewed changes

marcelbuesing added 7 commits November 15, 2019 12:27

Update to futures-preview 0.3.0-alpha.19 and async await syntax

ab6de34

Fix failing wunderlist and doc test

8336bb8

Start removal of pre 2018 edition source code artifacts

Gate async-await via feature flag

c1a28f7

Extract async_internal module

bafea1c

Remove rust-toolchain file

4338da8

Document async module, traits and fns

342498d

Make the reqwest and futures feature orthogonal.

8eb2d9d

This way reqwest and futures may also be used without each other.

marcelbuesing force-pushed the async-await branch from d9bc6c6 to 8eb2d9d Compare November 15, 2019 11:33

ramosbugs added 5 commits November 17, 2019 20:24

Make --all-features work + minor cleanup

2384c6a

Fix 1.34.0 build and other CI issues

284b64b

Fix cargo audit condition

cae392a

Disable futures-03 tests for rustc < 1.39

30ec6d7

Fix blocking http_client feature flags

4fe0ffe

Fix docs

6557a8a

ramosbugs merged commit e51417e into ramosbugs:master Nov 18, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update to futures 0.3.0 and async await syntax #71

Update to futures 0.3.0 and async await syntax #71

marcelbuesing commented Oct 21, 2019 •

edited

ramosbugs left a comment

ramosbugs commented Oct 22, 2019 •

edited

marcelbuesing commented Oct 22, 2019 •

edited

ramosbugs commented Oct 27, 2019

marcelbuesing commented Oct 28, 2019

ramosbugs commented Oct 28, 2019

marcelbuesing commented Nov 4, 2019

ramosbugs commented Nov 5, 2019

marcelbuesing commented Nov 7, 2019 •

edited

ramosbugs commented Nov 9, 2019

ramosbugs Nov 10, 2019

ramosbugs Nov 10, 2019

ramosbugs Nov 10, 2019

ramosbugs Nov 10, 2019

ramosbugs commented Nov 18, 2019

marcelbuesing commented Nov 18, 2019

Update to futures 0.3.0 and async await syntax #71

Update to futures 0.3.0 and async await syntax #71

Conversation

marcelbuesing commented Oct 21, 2019 • edited

ramosbugs left a comment

Choose a reason for hiding this comment

ramosbugs commented Oct 22, 2019 • edited

marcelbuesing commented Oct 22, 2019 • edited

ramosbugs commented Oct 27, 2019

marcelbuesing commented Oct 28, 2019

1.32.0

1.33.0

1.34.0

1.35.0

ramosbugs commented Oct 28, 2019

marcelbuesing commented Nov 4, 2019

ramosbugs commented Nov 5, 2019

marcelbuesing commented Nov 7, 2019 • edited

ramosbugs commented Nov 9, 2019

ramosbugs Nov 10, 2019

Choose a reason for hiding this comment

ramosbugs Nov 10, 2019

Choose a reason for hiding this comment

ramosbugs Nov 10, 2019

Choose a reason for hiding this comment

ramosbugs Nov 10, 2019

Choose a reason for hiding this comment

ramosbugs commented Nov 18, 2019

marcelbuesing commented Nov 18, 2019

marcelbuesing commented Oct 21, 2019 •

edited

ramosbugs commented Oct 22, 2019 •

edited

marcelbuesing commented Oct 22, 2019 •

edited

marcelbuesing commented Nov 7, 2019 •

edited