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

Make the rust hyper client Send so it can be used in rust threads more easily #19375

Merged
merged 11 commits into from
Aug 17, 2024

Conversation

wing328
Copy link
Member

@wing328 wing328 commented Aug 17, 2024

The hyper client cannot be used easily in threads as it isn't Send friendly. Something like this will fail to compile:

use petstore_hyper::apis::client::APIClient;
use petstore_hyper::apis::configuration::Configuration;

#[tokio::main]
async fn main() {
    tokio::spawn(async move {
        let client = APIClient::new(Configuration::new());
        let _result = client.pet_api().get_pet_by_id(5).await;
    });
}

Issue with more detailed steps to reproduce: #19208

This PR makes the hyper client and its internals Send. So the client can be used in threads.

It ended up being a little larger of a PR in the pursuit of writing a test. It turns out the tests aren't actually being run when invoking verify on the petstore samples. This PR had to touch a few other thing to ensure cargo test would work. Let me know if I should break it up into multiple PRs.

Should I update the pom.xml file to do a cargo test instead of a cargo check? I'm not sure if that was intentional.

Commit summary

This might be a good order to review the changes:

7ee4bca - This is the commit that actually makes the code work in threads. I adds the Send constraint to a bunch generic parameters and traits and uses an Arc instead of an Rc

bda9b6d - This commit adds a test to validate that it works and doesn't regress.

e53c04c - This commit regenerates the samples

The remaining commits were trying to get the tests to run in samples/client/others/rust/ to run successfully. There were several issues to fix to get tests to run successfully.

43ba4f0 46d3520 These set it up so the doc strings are compilable. The first one adds a property called externCrateName cribbing from the rust-server generator. The second one uses the externCrateName and fixes other issues with the example rustdoc examples.

ddb330c The reqwest tests were not compiling and needed a small change.

efbf445 This changes the petstore swagger file and generates the petstore tests to use https. http://petstore.swagger.io/v2 is 301 redirecting to https://petstore.swagger.io/v2 and this is breaking POSTS.
When the client recieves a redirect it does not resend the POST data, instead it switches to GET. This is in line with how browsers behave when encountering a 301 redirect on a POST request, so I wouldn't consider it a bug in the client, but instead a bug in the test code.

Pinging the rust technical committee members: @frol @farcaller @richardwhiuk @paladinzh @jacob-pro

PR checklist

  • Read the contribution guidelines.
  • Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed response from the community.
  • Run the following to build the project and update samples:
    ./mvnw clean package 
    ./bin/generate-samples.sh ./bin/configs/*.yaml
    ./bin/utils/export_docs_generators.sh
    
    (For Windows users, please run the script in Git BASH)
    Commit all changed files.
    This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
    These must match the expectations made by your contribution.
    You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example ./bin/generate-samples.sh bin/configs/java*.
    IMPORTANT: Do NOT purge/delete any folders/files (e.g. tests) when regenerating the samples as manually written tests may be removed.
  • File the PR against the correct branch: master (upcoming 7.6.0 minor release - breaking changes with fallbacks), 8.0.x (breaking changes without fallbacks)
  • If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

blazzy and others added 11 commits July 17, 2024 15:12
This is follows the lead of the rust hyper server generator and provides
an externCrateName. This is because the crate name used for importing
can be different from the package name, because dashes `-` get converted
to underscores `_`.

This allows us to write example code in rustdoc that compiles
successfully.
http://petstore.swagger.io/v2 is 301 redirecting to
https://petstore.swagger.io/v2 and this is breaking posts to the API.
When the client recieves a redirect it does not resend the POST data,
instead it switches to GET. This is in line with how browsers behave
when encountering a 301 redirect on a POST request.
This trait is also helpful in making the api work well with threads.
@wing328 wing328 changed the title Blazzy hyper client send Make the rust hyper client Send so it can be used in rust threads more easily Aug 17, 2024
@wing328 wing328 added this to the 7.8.0 milestone Aug 17, 2024
@wing328 wing328 marked this pull request as ready for review August 17, 2024 08:01
@wing328 wing328 merged commit bb831da into master Aug 17, 2024
22 checks passed
@wing328 wing328 deleted the blazzy-hyper-client-send branch August 17, 2024 08:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants