cvat-core: retry HTTP requests if a 429 status is returned

[SpecLad]

Motivation and context

This should help reduce breakage if a user hits an API rate limit.

How has this been tested?

Manual testing.

Checklist

• I submit my changes into the develop branch
• I have created a changelog fragment
• [ ] I have updated the documentation accordingly
• [ ] I have added tests to cover my changes
• [ ] I have linked related issues (see GitHub docs)
• [ ] I have increased versions of npm packages if it is necessary
  (cvat-canvas,
  cvat-core,
  cvat-data and
  cvat-ui)

License

• I submit my code changes under the same MIT License that covers the project.
  Feel free to contact the maintainers if that's a concern.

[bsekachev]

We also have a worker cvat-core/src/download.worker.ts to download data chunks in another thread
Will this plugin have effect inside the worker?

[bsekachev]

If error.response.headers['retry-after'] is supposed to be a number or not a number string:

Suggested change

	retryDelay: (retryCount, error) => {
	const retryAfterValue = error.response.headers['retry-after'];
	// Retry-After is allowed to be a number as well as a date.
	// We're probably not going to use the latter option, though, so don't bother parsing it.
	if (!retryAfterValue || !/^\d+$/.test(retryAfterValue)) return axiosRetry.exponentialDelay(retryCount, error);
	return parseInt(retryAfterValue, 10) * 1000;
	},
	retryDelay: (retryCount: number, error: Error): number => {
	// Retry-After is allowed to be a number as well as a date.
	// We're probably not going to use the latter option, though, so don't bother parsing it.
	const retryTimeout = (+error.response.headers['retry-after']) * 1000;
	if (Number.isNaN(retryTimeout)) {
	return axiosRetry.exponentialDelay(retryCount, error);
	}
	return retryTimeout;
	},

[SpecLad]

By "number" I really mean "non-negative decimal integer". See the definition of Retry-After.

Blindly converting to a JavaScript number could yield undesirable values like negative numbers or infinity, so I'd rather not do that.

[bsekachev]

For avoid infinity/floats we may use:

Number.isInteger(retryTimeout) && retryTimeout > 0

Instead of isNaN

[bsekachev]

It is a little bit more pretty, than regex using

[SpecLad]

It is a little bit more pretty, than regex using

I don't know about pretty, but I find my version more straightforward. I verify that:

1. The header is present.
2. The header value follows the expected grammar.

Then I convert the value to the expected type.

Your version relies on JS's numeric conversion doing the right thing for values not matching the grammar, which isn't even true:

> +'0xA'
10

[bsekachev]

Why not true? You just converted HEX 0xA to decimal 10.
And this is correct.

[bsekachev]

Let's check with regex any data come from the server then...?

[bsekachev]

But we do not expect +55 from the server, right? As well as 0xA..

[SpecLad]

Why not true? You just converted HEX 0xA to decimal 10.
And this is correct.

It's not correct, because hexadecimal is not allowed by the grammar.

Let's check with regex any data come from the server then...?

Sure? If it's amenable to such validation, then why not.

What's your point? Plus signs are not allowed by the grammar, so this string should be rejected - and that's what my implementation does.

But we do not expect +55 from the server, right? As well as 0xA..

We don't - and that's why I reject such values.

[bsekachev]

My point is that your example with 0xA is not real. If server returns '0xA' - server does not work correctly.
As if server returns '+55' - it is valid number, but the regular experssion rejects it. But this value also is not real.
IMHO we should not use regex to validate numbers, it looks ugly in the code.

It is up to you how implement, you asked for a review, I reviewed.

[bsekachev]

But to be honest, looking at the worker now, I do not remember why it was added.
And I have some doubts we need it, need to check additionally

[SpecLad]

But to be honest, looking at the worker now, I do not remember why it was added. And I have some doubts we need it, need to check additionally

Well, you tell me if you want me to add retry code to the worker. 🤷‍♂️

[bsekachev]

Well, you tell me if you want me to add retry code to the worker. 🤷‍♂️

First of all need to check that this retry logic has effect inside the worker, if it does not, so, please add the code there.
In the future somebody will check if we really need this worker.

[SpecLad]

First of all need to check that this retry logic has effect inside the worker, if it does not, so, please add the code there.

Okay, I'll do that.

(FWIW, I'm pretty sure the current code will not affect the worker, since the worker doesn't import server-proxy.ts.)

[SpecLad]

I updated the code to share basic configuration (including retries) between the worker and server-proxy.ts.

[codecov]

Codecov Report

Merging #7216 (559a3b8) into develop (571e622) will decrease coverage by 0.23%.
Report is 1 commits behind head on develop.
The diff coverage is 50.00%.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #7216      +/-   ##
===========================================
- Coverage    81.54%   81.31%   -0.23%     
===========================================
  Files          365      367       +2     
  Lines        39261    39364     +103     
  Branches      3631     3646      +15     
===========================================
- Hits         32016    32010       -6     
- Misses        7245     7354     +109     

Components	Coverage Δ
cvat-ui	74.97% <50.00%> (-0.45%)	⬇️
cvat-server	87.09% <ø> (-0.01%)	⬇️