Skip to content

connect: expire the timeout when trying next#11935

Closed
bagder wants to merge 1 commit into
masterfrom
bagder/expire-ballers
Closed

connect: expire the timeout when trying next#11935
bagder wants to merge 1 commit into
masterfrom
bagder/expire-ballers

Conversation

@bagder
Copy link
Copy Markdown
Member

@bagder bagder commented Sep 25, 2023

... so that it gets called again immediately and can continue trying addresses to connect to. Otherwise it might unnecessarily wait for a while there.

Fixes #11920
Reported-by: Loïc Yhuel

Copy link
Copy Markdown
Contributor

@icing icing left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does not hurt, since Curl_expire() is idempotent when called several times.

@hwti
Copy link
Copy Markdown
Contributor

hwti commented Sep 25, 2023

This doesn't always work in my tests.

Curl_expire returns in the "sooner" ( if(diff > 0) ) case, with nowp pointing to the current timeout (which triggered the curl_multi_perform and will be removed at the end). I wonder if this is expected.
And after this, multi_timeout can either :

  • find the newly added timer and returns 0
  • find the next timer (the connection timeout in my case)

@bagder
Copy link
Copy Markdown
Member Author

bagder commented Sep 25, 2023

Yes, on repeated runs it shows for me as well. A premature PR...

@bagder
Copy link
Copy Markdown
Member Author

bagder commented Sep 25, 2023

@hwti okay, how about this? I actually poked on Curl_expire now as I believe the "optimization" in there could get it wrong.

@hwti
Copy link
Copy Markdown
Contributor

hwti commented Sep 25, 2023

@bagder The commit seems to avoid the issue, but the current timeout remains in the list until the next curl_easy_perform, since add_next_timeout is no longer called on the current call.

I have another solution in add_next_timeout : #11937.
It might fix more cases : if there are two similar timeouts, when the first one is serviced the second one might expire in less than 1ms, and would currently be removed from the list.
With this one, I believe the Curl_expire optimization can be kept.

@hwti
Copy link
Copy Markdown
Contributor

hwti commented Sep 25, 2023

I created #11938 and #11939 to reduce the number of useless wakeups.
The two successive curl_multi_perform were sometimes hiding the issues, so this could help detecting them, but also break cases which appeared to work most of the time.

... so that it gets called again immediately and can continue trying
addresses to connect to. Otherwise it might unnecessarily wait for a
while there.

Fixes #11920
Reported-by: Loïc Yhuel
@bagder bagder force-pushed the bagder/expire-ballers branch from 0d66f54 to 985bb6d Compare September 25, 2023 23:02
@bagder
Copy link
Copy Markdown
Member Author

bagder commented Sep 25, 2023

I believe the Curl_expire optimization can be kept.

I believe so as well. I removed the commit again from this PR.

@bagder bagder closed this in 01d8473 Sep 27, 2023
@bagder bagder deleted the bagder/expire-ballers branch September 27, 2023 08:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

Random delay when trying the next IP after a connect timeout

3 participants