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

Allow a connection attempt to timeout and keep trying resolved addresses #618

Merged
merged 1 commit into from
Dec 9, 2023

Conversation

ex-nihil
Copy link
Contributor

Before this change connect_host only attempted to connect to the first resolved SocketAddr.

My interpretation was that this was the indented use of agent.config.timeout_connect but, I'm unsure as the default value seems to be 30 sec.

Breaking change for any projects depending on the timeout_connect config, maybe introduce a separate configuration for this timeout.

@algesten
Copy link
Owner

algesten commented May 5, 2023

The intention is that connect_timeout is regardless of how many SocketAddr there might be.

Is your problem that the failing socket address takes longer than 30 seconds to fail?

@ex-nihil
Copy link
Contributor Author

ex-nihil commented May 5, 2023

My problem is that there's no SocketAddr failover.
Whatever connect_timeout is set to will be the amount of time we wait for the first SocketAddr connection attempt.

And when that fails the request is aborted by this line.

Some(deadline) => Some(time_until_deadline(deadline)?),

edit: My first address never responds

@algesten
Copy link
Owner

algesten commented May 5, 2023

edit: My first address never responds

As far as I know all OSes will timeout socket connection attempts at some point. Some quick googling seems to say for windows it's 72 seconds and for Linux 60 seconds. In many cases I would image the timeout to be shorter. For example, if the host is there, but port closed, the SYN would be responded with an RST and the fail be instant.

The 30 second value is for the overall the connection attempt, not per socket addr. On your host, could you try lowering the OS default timeout and see if that helps?

I wonder what cURL does here, does it fail faster than the OS so it can try more addresses?

@ex-nihil
Copy link
Contributor Author

ex-nihil commented May 5, 2023

I wonder what cURL does here, does it fail faster than the OS so it can try more addresses?

Curl failed over pretty much instantly when it was happening.

It seems the Spotify server is no longer non-responsive for the first host, so unable to recreate right now.
I will try to setup a test the behavior later tonight.

@ex-nihil
Copy link
Contributor Author

ex-nihil commented May 5, 2023

Added a testcase to this PR socket_addr_fail_over recreating it. Your suggestion with lowering the OS timeout should absolutely work, but the question is how curl was able to figure it out.

I would need to setup a DNS server to compare.

@ex-nihil
Copy link
Contributor Author

ex-nihil commented May 5, 2023

Well, found why curl worked. It attempts the first two entries at the same time.
https://github.com/curl/curl/blob/faebcee349d45f46638add6dda3185425c7afac6/lib/connect.c#L752

They also set the addr connection attempt to timeoutms/2 if there's multiple addresses.
https://github.com/curl/curl/blob/faebcee349d45f46638add6dda3185425c7afac6/lib/connect.c#L504

@algesten
Copy link
Owner

algesten commented May 5, 2023

Right. That seems reasonable. We could do that too. if there are at least two addresses, split connect_timeout in 2 and try both after each other.

@ex-nihil
Copy link
Contributor Author

ex-nihil commented May 5, 2023

Updated the PR.
The behavior for multiple addresses is now that a socket connection gets half of the remaining time until the deadline.
So as an example a timeout of 10sec with 3 resolved SocketAddr will result in.

Timeouts
1st after 5 sec
2nd after 2.5 sec
3rd after 1.25 sec

@algesten algesten merged commit 9228547 into algesten:main Dec 9, 2023
@algesten
Copy link
Owner

algesten commented Dec 9, 2023

Let's land this.

Thanks!

@algesten algesten mentioned this pull request Dec 9, 2023
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.

2 participants