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 --no-keepalive (curl option) #3253

Open
fabricereix opened this issue Sep 23, 2024 · 12 comments · May be fixed by #3398
Open

Add option --no-keepalive (curl option) #3253

fabricereix opened this issue Sep 23, 2024 · 12 comments · May be fixed by #3398
Labels

Comments

@fabricereix
Copy link
Collaborator

By default, Hurl keeps connections alive between requests (like curl).
It might be useful to disable it.

I've tested an API endpoint that didn't return any content (HTTP 204).
In order to get the response content (HTTP 200), I needed to disable the keep-alive (and force HTTP1.1 over HTTP2)

@fabricereix fabricereix added the enhancement New feature or request label Sep 23, 2024
@fabricereix
Copy link
Collaborator Author

In fact, I needed to add a delay to get a correct response from my API endpoint.
This option might still be useful but clearly low priority.

@jcamiel jcamiel added the good first issue Good for newcomers label Oct 30, 2024
@theoforger
Copy link

Hello @jcamiel ! I'd love to work on this feature!

I assume curl::easy::Easy::tcp_keepalive is what I should use? But according to the doc it already defaults to false. Am I missing anything here?

Thanks!

@jcamiel
Copy link
Collaborator

jcamiel commented Nov 12, 2024

@theoforger I've the impression that the right option to disable reusing connection is not CURLOPT_TCP_KEEPALIVE, but something like CURLOPT_FRESH_CONNECT or CURLOPT_FORBID_REUSE. I've to dig to understand the difference but CURLOPT_TCP_KEEPALIVE is more about sending tcp keepalive probe and is independent on connection reuse or not.

@theoforger
Copy link

@jcamiel Thanks for the info!

I did some testing on all three options:

    let mut easy = Easy::new();
    easy.url("http://127.0.0.1:8080")?;

    easy.fresh_connect(false)?; // CURLOPT_FRESH_CONNECT
    easy.tcp_keepalive(true)?; // CURLOPT_TCP_KEEPALIVE
    easy.forbid_reuse(false)?; // CURLOPT_FORBID_REUS

    easy.perform()?;
    println!("Response code: {}", easy.response_code()?);

while monitoring the connections with ss -tnp.

For some reasons no matter what options I use, libcurl doesn't keep any connection open afterwards. What's happening here?

@jcamiel
Copy link
Collaborator

jcamiel commented Nov 12, 2024

It think it may also depends on the HTTP version protocol. I don't think there is any reuse with a server responding HTTP/1.0, you should try with a "known" host like https://google.com

@theoforger
Copy link

You're right. It's working now!

My understanding is that:

Either of them can stop reuse. Do we want to combine them or keep them as two separate flags?

Also is there still an interest for --no-keepalive?

@jcamiel
Copy link
Collaborator

jcamiel commented Nov 12, 2024

Don't think there is a need to keepalive / no-keepalive. We can begin to implement CURLOPT_FRESH_CONNECT (--fresh-connect) and that should be sufficient to expose an option to be able to not reuse connection.

@jcamiel
Copy link
Collaborator

jcamiel commented Nov 12, 2024

@fabricereix one thing to note it that there is no curl cli equivalent of CURLOPT_FRESH_CONNECT, it's just a libcurl option.

@theoforger
Copy link

Hello again! I just filed a PR. I mainly used #2468 as a reference to work on this. Let me know if there's any changes you wish to make 🙏

@fabricereix
Copy link
Collaborator Author

This --no-keepalive curl option does not seem in fact straightforward.
We would need a test for which you could see a difference with and without this option, so that we could also validate with Hurl.
Contrary to what I thought initially, it does not seem to be directly related to reusing connection.

@theoforger
Copy link

@jcamiel I would still love to contribute though. If there's anything I can work on, feel free to let me know! 🙏

@jcamiel
Copy link
Collaborator

jcamiel commented Nov 14, 2024

@theoforger Of course, let us just check something that will be merged without problem, so we don't waste your time this time!

@jcamiel jcamiel removed the good first issue Good for newcomers label Nov 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants