Skip to content

Conversation

@srenauld
Copy link

This commit contains modifications to the Rust codegen module in order to be able to generate futures-aware, threadsafe reqwest bindings from an openAPI template, in order to be able to take advantage of non-blocking API calls.

The changes are as follows:

  • Added a library reqwestFutures
  • Dependency changes in Cargo.toml (futures is now a dependency for reqwest when using this library
  • Due to the requirement that all futures should be Send, the reference-counting pointer is now an Arc
  • Templates, evidently. They're very similar to the reqwest module itself, as the API from the sync and async client are virtually the same

Due to the upcoming changes regarding async/await, I took the deliberate decision of calling the library reqwestFutures rather than reqwestAsync. When Rust 1.39 lands and those features become stable, a conversion tool from one to the other should be easy to build.

Rust committee members are @frol @farcaller @bjgill @richardwhiuk - since this is my first PR here, I guess mentioning them should be good enough as a CC?

This commit contains modifications to the Rust codegen module in order
to be able to generate futures-aware, threadsafe `reqwest` bindings
from an openAPI template, in order to be able to take advantage
of non-blocking API calls.
@bcourtine
Copy link
Contributor

Hi @srenauld ,

This PR looks interesting.

A technical point: since it is a new generator library, you should add a generation code for this library in the bin/rust-petstore.sh script, and also commit generated files (which will be checked by the continuous integration).

Personnaly, I was waiting Rust 1.39 to use reqwestAsync, but it is just a personal opinion. ;)

@srenauld
Copy link
Author

Hey @bcourtine !

Thank you for the feedback.

I deliberately left that name so we can have both; for instance, I commonly work with processor architectures where rust 1.39 is a no-go but futures are perfectly fine, and as such, would benefit from having the option to have either. That's why I laid it out the way I did. After all, I'm also pretty sure I am not the only one in this case.

I will add a generation code and some tests to cover everything. I wasn't sure what I had to do next - now I know :-)

I was not aware that openapi-generator could generate multipart MIME bindings.
Implementing them was complicated further by `reqwest::async::multipart::Form` having
an entirely different API than the `sync` version of it, leading to two decisions:

1. The file is read in its entirety. This is the same behavior as on the sync version;
if time allows, I will convert this into a chunked stream
2. The entire mime detection logic is taken from the `sync` version and added as a
private function in the module
@srenauld
Copy link
Author

Hey @bcourtine,

I added a full client sample, which also revealed a feature I was not aware of - MIME file uploads. That's also done now.

Throughout this time I've struggled with some teething CI problems, including:

  • timeouts on drone
  • issues on travis such as the following: Could not transfer artifact org.openapitools:openapi-generator-maven-plugin:pom:4.1.3-SNAPSHOT from/to sonatype-snapshots (https://oss.sonatype.org/content/repositories/snapshots/): Failed to transfer file https://oss.sonatype.org/content/repositories/snapshots/org/openapitools/openapi-generator-maven-plugin/4.1.3-SNAPSHOT/openapi-generator-maven-plugin-4.1.3-SNAPSHOT.pom with status code 502
  • An unknown issue regarding the R generator on circleCI

Is there any chance you could have a quick look and tell me if there's something obvious that I am missing and/or re-run the CI to see if the travis error was just intermittent? Would be greatly appreciated :-)

@ctaggart
Copy link

It looks to me like this pull request address #3865, but should probably be closed in favor of #4210 which will update to reqwest 0.10 once that ships.

@srenauld
Copy link
Author

@ctaggart As explained in the initial PR comment, I'd personally be in favor of keeping both (as in, pre-1.39 futures-based, and post-1.39 async/await), even if the futures-based implementation is on life support.

Let me give you an example. In the past three years I've built, developed and released a few products running on a processor from ARM that you literally cannot use anything beyond the nightly from november 30th 2018 with. The reason being that a breaking change between 1.32 and 1.33 happened - I never properly dug down why, but I know it is related to atomic operation emulation (something which this specific processor needs).

For that reason, even though the async/await version is "newer", it is also inaccessible to those clients, even though old-school futures themselves are, as is a lot of the Rust ecosystem. And for that reason, for such requirements where one cannot use the mainline, greenest possible compiler, I would personally be in favor of doing both.

@wing328
Copy link
Member

wing328 commented Jun 16, 2020

@srenauld I've added an option to support async operations in the Rust reqwest client.

I wonder if you can pull the latest master to give it a try.

@wing328
Copy link
Member

wing328 commented Jan 25, 2021

Closing as there's no update.

@wing328 wing328 closed this Jan 25, 2021
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.

4 participants