tool: add a retry delay for all transfers to the same host on http error 429 by htasta2 · Pull Request #21355 · curl/curl

htasta2 · 2026-04-17T10:46:19Z

Don't start new transfers to the same host if this host has returned a 429 error.

The --retry-delay setting is used for all transfers to the same host on error 429.

vszakats

I suggest moving the 3 new functions within tool_operate.c as static ones.

And remove the double newlines (nit).

htasta2 · 2026-04-17T11:58:51Z

I suggest moving the 3 new functions within tool_operate.c as static ones.

I put them in there because of this sentence in the CONTRIBUTE.md

If you bring completely new functionality, try writing it in a new source file.

I'll move them.

And remove the double newlines (nit).

What exactly do you mean with this? What is "nit"?

vszakats · 2026-04-17T12:02:52Z

What exactly do you mean with this? What is "nit"?

It's a minor issue, e.g. whitespace.

htasta2 · 2026-04-17T12:34:24Z

It's all in tool_operate.c now and no more double new line between functions.

htasta2 · 2026-04-20T17:04:49Z

It's using origin now instead of host as per your request.

htasta2 · 2026-04-21T09:03:06Z

Issues fixed.

Can I pull the http response out of the if/else or is there a deeper reason why it's declared in both those blocks instead of outside?

bagder · 2026-04-26T21:54:42Z

It is helpful if you select "resolve conversation" for the remarks you think are are addressed. Otherwise it looks like all remarks are still pending...

htasta2 · 2026-04-27T07:43:06Z

Resolved all conversations and it's using curl_msnprintf() now.

No checks for the return value of this function as I've seen it done plenty of times in other places like that.

bagder

A primary reason for the revert was the complaint on lifetime management. The 429 list may live longer than the individual transfers so the origin pointers might then point to freed memory. Won't they?

bagder · 2026-06-01T21:46:09Z

+  goto clean;
+
+urlerr:
+  origin = curlx_strdup("-/-/-");


Can you elaborate on why it sets a dummy origin if one of the URL API functions fail? When is that useful?

It's useful when the URL is invalid. This will be handled at a later stage and with a dummy it will get to that later stage where other valid URLs will still be processed.

I think this needs a test case to prove that this is the best way. It does not make sense to me.

htasta2 · 2026-06-02T11:01:12Z

A primary reason for the revert was the complaint on lifetime management. The 429 list may live longer than the individual transfers so the origin pointers might then point to freed memory. Won't they?

The list is cleaned up right after the transfers are cleaned up, I don't see where the list items would be used after the transfers are over.

Closes curl#21355

htasta2 · 2026-06-08T12:59:04Z

I changed it so the list is using a copy of the origin from the transfers.
This way the list could be used even after all transfers have completed and freed.

Copilot

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

bagder

We need a test or two for this feature to make sure it works now and to make sure we don't accidentally break it the future.

bagder · 2026-07-01T20:56:56Z

+  goto clean;
+
+urlerr:
+  origin = curlx_strdup("-/-/-");


I think this needs a test case to prove that this is the best way. It does not make sense to me.

bagder · 2026-07-01T20:58:11Z

      break;

+    /* store the origin in the per transfer struct for 429 delay checks */
+    result = set_per_transfer_origin(per->url, &per->origin);


Why create this unconditionally and use lots of allocations for all transfers unconditionally when it is only needed for 429 responses? Why not wait and create the origin once and if it is needed?

If I return an error at this point instead of creating a generic host/port/scheme curl will exit with the error instead of processing the valid URL after the invalid one.

For example if I give a list like this:

https://example.com
htps:exampl.com
http://not-an-example.com

Nothing will be downloaded despite 2 of the 3 URLs being valid.

I created this unconditionally because otherwise the URL will be parsed all the time. I had it initially like that, I can go back to that one.

If I have to redo this, then I also have to update the translate of URL errors to general curl errors as I ran into such an issue when I had it like that. So please tell me clearly which way you want it so I don't have to then redo it again.

I created this unconditionally because otherwise the URL will be parsed all the time.

Why would it? if you get a 429 and ->origin is a NULL, you can populate the ->origin then. If it would come back to that condition later, ->origin would already be populated and you don't need to parse the URL again.

The point being that you avoid URL parsing for all the URLs that never get a 429.

Oh I understand now. So still use the per struct but only fill it on 429 error. Yeah, that makes sense. I didn't use the per struct at all before that's why there was a ton of URL parsing which rubbed me the wrong way.

I will change it that way, but as I said to do this properly and to get rid of the "-/-/-" thingy which rubbed you the wrong way 😝 I need to redo the URLerror to CURLerror translation function because some errors are not translated properly and just result in CURLE_OK.

bagder · 2026-07-01T21:00:07Z

+  len_origin = strlen(host) + strlen(port) + strlen(scheme) + 3;
+  origin = curlx_malloc(len_origin);


avoid strlen, malloc and snprintf by:

Suggested change

len_origin = strlen(host) + strlen(port) + strlen(scheme) + 3;

origin = curlx_malloc(len_origin);

origin = curl_maprintf("%s/%s/%s", host, port, scheme);

if(!origin) {

You told me to use msnprintf, that's why I used it.

what do you think about using curl_msnprintf() instead to avoid three memcpy()s ?

What about this in docs/examples/.checksrc:

# Use of curl printf functions is discouraged
banfunc curl_maprintf

What about this in docs/examples/.checksrc:

This applies to example code. It's built differently and reflects
different practices than core/test code.

You told me to use msnprintf, that's why I used it.

Then the conditions were different or I didn't understand what you were asking. We prefer avoiding "raw" allocs + fiddling and rather use functions that handle it themselves.

htasta2 · 2026-07-01T22:58:23Z

As for the test. We discussed this in the IRC and it's non trivial to test this.

This is why I wrote a small test script for myself on my server and local machine to check if the behaviour is how I expect it to be.

bagder · 2026-07-02T07:17:06Z

As for the test. We discussed this in the IRC and it's non trivial to test this.

We still need tests. I think its worth spending some extra effort on that. A test that first returns a 429 and then a 200 shouldn't be too hard to add and would at least confirm that the basics do what they're supposed to do.

htasta2 · 2026-07-02T08:46:09Z

As for the test. We discussed this in the IRC and it's non trivial to test this.

We still need tests. I think its worth spending some extra effort on that. A test that first returns a 429 and then a 200 shouldn't be too hard to add and would at least confirm that the basics do what they're supposed to do.

That doesn't properly test it. The test needs to check if others are still going through while the 429 one is blocked for a certain amount of time otherwise the code isn't really tested. Because of the async and parallel nature of it, the output of the test isn't trivial to check.

I'll be gone for the weekend, but after that I can show how I "manually" tested it so far. Maybe you can then add a testcase to your server that will properly test this, I wasn't able to get anything done with the existing functions, I tried several months ago.

bagder · 2026-07-02T10:59:08Z

That doesn't properly test it.

It would be a first miniature attempt to at least exercise some of the code. I won't stop you from doing better ones.

…ror 429

the list's lifetime is tied to the pre->transfers, so when the origin living in per-transfer will be freed the list is freed too.

The HTTP error 429 delay list needs some extra allocs. The limit is increased in tests: 142,440,445,767

… are freed

…9 error happens

bagder

Still concerned about the lack of tests for this.

@icing any thoughts on how we can make a test case for this?

bagder · 2026-07-29T14:45:26Z

+
+    /* The retry delay for 429 applies to all requests to the same origin.
+       Update the global 429 delay list for this origin. */
+    if(http_error == 429) {


If curl is not doing parallel downloads, this PR does nothing for the 429 responses so it seems unnecessary to add them to a list that is never used. Doesn't it?

bagder · 2026-07-29T14:46:04Z

+      if(item && (time(NULL) < item->startat)) {
+        per->startat = item->startat;
+        per->added = FALSE;
+        sleeping = TRUE;


Should this also log something to the output to help explain to users what's going on ?

bagder · 2026-07-29T14:46:35Z

 </protocol>
 <limits>
-Allocations: 180
+Allocations: 190


None of these Allocations changes should be needed anymore, right?

htasta2 · 2026-07-31T08:18:33Z

They way it currently is, is broken.

It is how I initially said. Either you use memory or you do the calculation every time.

Now it's only using memory when the error happens but all the transfers who are about to fire do not have the information and therefore fire away. So every transfer still needs the URL to be parsed and then checked against the list for it to work this way.

As I said, I don't want to run from Pontius to Pilatus and back all the time, that's why I didn't change anything further.
We need to talk this through on the IRC so the mechanism is really understood and I get the go for the actual solution.

bagder · 2026-10-23T08:32:00Z

There's been almost three months and this now merge-conflicts. Are you still working on this @htasta2 ? Please do not think that conversations on IRC is better than here. They are ephemeral and hard for people to join in, while discussions here stick to this particular PR.

bagder · 2026-10-29T08:21:03Z

Abandoned

github-actions Bot added the cmdline tool label Apr 17, 2026

vszakats requested changes Apr 17, 2026

View reviewed changes

htasta2 force-pushed the 429-delay branch 2 times, most recently from e546506 to 61ea829 Compare April 17, 2026 12:33

vszakats reviewed Apr 17, 2026

View reviewed changes

Comment thread src/tool_operate.c Resolved

htasta2 force-pushed the 429-delay branch 2 times, most recently from a048e91 to f69d70b Compare April 17, 2026 18:48

vszakats reviewed Apr 19, 2026

View reviewed changes

Comment thread src/tool_operate.c Resolved

bagder reviewed Apr 20, 2026

View reviewed changes

Comment thread src/tool_operate.c Resolved

Comment thread src/tool_operate.c Resolved

Comment thread src/tool_operate.c Resolved

Comment thread src/tool_operate.c Resolved

Comment thread src/tool_operate.c Resolved

htasta2 force-pushed the 429-delay branch from f69d70b to 757e446 Compare April 20, 2026 17:01

htasta2 requested a review from vszakats April 20, 2026 17:05

htasta2 force-pushed the 429-delay branch from 757e446 to ae8d01b Compare April 20, 2026 17:09

htasta2 requested a review from bagder April 20, 2026 17:15

htasta2 force-pushed the 429-delay branch from ae8d01b to 58b0bb2 Compare April 20, 2026 21:07

bagder reviewed Apr 21, 2026

View reviewed changes

Comment thread src/tool_operate.c Resolved

Comment thread src/tool_operate.c Resolved

Comment thread src/tool_operate.c Resolved

Comment thread src/tool_operate.c Resolved

htasta2 force-pushed the 429-delay branch 3 times, most recently from fd2ec2f to 3098f22 Compare April 21, 2026 08:53

htasta2 requested a review from bagder April 21, 2026 09:11

bagder reviewed Apr 26, 2026

View reviewed changes

Comment thread src/tool_operate.c Resolved

htasta2 force-pushed the 429-delay branch 2 times, most recently from 4de7304 to 9c93701 Compare April 27, 2026 07:38

htasta2 requested a review from bagder April 27, 2026 07:43

bagder reviewed Jun 1, 2026

View reviewed changes

outcast36 pushed a commit to greearb/curl that referenced this pull request Jun 3, 2026

tool: add a retry delay for transfers to same origin on 429

b24d867

Closes curl#21355

htasta2 force-pushed the 429-delay branch from 4543154 to bcf30a7 Compare June 8, 2026 12:17

htasta2 force-pushed the 429-delay branch from bcf30a7 to dcd084f Compare June 23, 2026 09:08

icing requested a review from bagder June 29, 2026 13:35

bagder requested a review from Copilot July 1, 2026 14:53

Copilot started reviewing on behalf of bagder July 1, 2026 14:53

View session

Copilot AI reviewed Jul 1, 2026

View reviewed changes

Comment thread src/tool_operate.c Resolved

bagder reviewed Jul 1, 2026

View reviewed changes

htasta added 8 commits July 6, 2026 10:42

tool: add a retry delay for all transfers to the same host on http er…

c8908b6

…ror 429

tool: use origin instead of host for 429 delay identification

262a2c8

tool: dont alloc extra space for origins in the 429 delay list

c494e9e

the list's lifetime is tied to the pre->transfers, so when the origin living in per-transfer will be freed the list is freed too.

tool: fixed double closing of file handle bug

87af4d5

tests: adjusted the alloc limit in tests

2e59f65

The HTTP error 429 delay list needs some extra allocs. The limit is increased in tests: 142,440,445,767

tool: copy origin for 429 list so it can be used even after transfers…

8c31029

… are freed

tool: User curl_maprintf to create host/port/scheme for 429 list

07e02f9

tool: only populate the origin field of the per_transfer struct if 42…

5625eec

…9 error happens

htasta2 force-pushed the 429-delay branch from dcd084f to 5625eec Compare July 6, 2026 18:53

bagder reviewed Jul 29, 2026

View reviewed changes

bagder added the needs-info label Oct 23, 2026

bagder closed this Oct 29, 2026

		len_origin = strlen(host) + strlen(port) + strlen(scheme) + 3;
		origin = curlx_malloc(len_origin);

Conversation

htasta2 commented Apr 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vszakats left a comment

Choose a reason for hiding this comment

Uh oh!

htasta2 commented Apr 17, 2026

Uh oh!

vszakats commented Apr 17, 2026

Uh oh!

htasta2 commented Apr 17, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

htasta2 commented Apr 20, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

htasta2 commented Apr 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bagder commented Apr 26, 2026

Uh oh!

Uh oh!

htasta2 commented Apr 27, 2026

Uh oh!

bagder left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

htasta2 commented Jun 2, 2026

Uh oh!

htasta2 commented Jun 8, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

bagder left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

htasta2 Jul 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

htasta2 Jul 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vszakats Jul 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

htasta2 commented Apr 17, 2026 •

edited

Loading

htasta2 commented Apr 21, 2026 •

edited

Loading

htasta2 Jul 1, 2026 •

edited

Loading

htasta2 Jul 1, 2026 •

edited

Loading

vszakats Jul 2, 2026 •

edited

Loading

htasta2 commented Jul 1, 2026 •

edited

Loading