Orange-OpenSource / hurl

Hurl, run and test HTTP requests with plain text.
https://hurl.dev
Apache License 2.0
13.17k stars 492 forks source link

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

Open theoforger opened 1 week ago

theoforger commented 1 week ago

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 commented 1 week ago

Hi @theoforger

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

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);

Thanks!!

theoforger commented 1 week ago

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

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

jcamiel commented 1 week ago

@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 commented 1 week ago

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 commented 1 week ago

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

jcamiel commented 1 week ago

Sorry again for this 😔