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

Add option --fresh-connect to optionally disable connection reuse #3398

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

theoforger
Copy link

Closes #3253.

What

This PR implements the --fresh-connect option, which makes sure a new connection is created for each request. Therefore not reusing any existing connections. It also includes necessary changes in docs and test code.

How

The --fresh-connect flag interfaces with libcurl and set the CURLOPT_FRESH_CONNECT option to true.

@jcamiel
Copy link
Collaborator

jcamiel commented Nov 13, 2024

Hi @theoforger

Thanks for the PR, just a couple of things to change:

  • integration tests are failing: if we look at the code to configure the HTTP client

https://github.com/Orange-OpenSource/hurl/blob/master/packages/hurl/src/http/client.rs#L394-L409 we can see that

self.handle.fresh_connect(true)?; is called in certain conditions. We have to do this when user wants to change the HTTP version of a request on the fly. libcurl tries really hard to reuse existing connection, and we have to reset the connection to change the HTTP version. It corresponds to this file:

GET https://foo.com
[Options]
http2: true

GET https://bar.com
[Options]
http3: true

I think we can fix this just by moving self.handle.fresh_connect(options.fresh_connect)?; before self.state.set_requested_http_version(http_version);

  • a small detail, in fresh_connect.option, we can add cli_only: true. In the docs, the option will display This is a cli-only option., indicating that we cannot, for the moment, set the option in [Options] section.

Thanks!!

@theoforger
Copy link
Author

theoforger commented Nov 13, 2024

The requested changes are pushed. Thanks for the explanation! 🙏

Edit: Forgot to sign my commits. Will work on that shortly.

@jcamiel
Copy link
Collaborator

jcamiel commented Nov 13, 2024

@theoforger we're discussing with the other maintainers whether we keep this option or not. There is indeed --keepalive / no-keepalive, but there isn't a curl equivalent to --no-fresh-connect (only a libcurl). I'm very very sorry you may have worked a lot for nothing but we don't even know how we can make an integration test with it (integration tests are here https://github.com/Orange-OpenSource/hurl/tree/master/integration). We can keep the PR open but there is a good chance we're going to postpone it. Once again, sorry, it's me that have proposed something that I should have discuss with the other maintainers.

@fabricereix
Copy link
Collaborator

The initial reason to implement --no-keepalive was because it was an existing curl option. I'm not convinced to add this new option to Hurl if it is not in curl.

@theoforger
Copy link
Author

@jcamiel @fabricereix It happens. No worries! Thanks for this great learning opportunity 🙏

@jcamiel
Copy link
Collaborator

jcamiel commented Nov 13, 2024

Sorry again for this 😔

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.

Add option --no-keepalive (curl option)
3 participants