| Message ID | 20240422142547.281064-5-derek.buitenhuis@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | HTTP rate limiting and retry improvements | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
On Mon, 22 Apr 2024, Derek Buitenhuis wrote: > 429 and 503 codes can, and often do (e.g. all Google Cloud > Storage URLs can), return a Retry-After header with the error, > indicating how long to wait, in seconds, before retrying again. > If it is not respected by, for example, using our default backoff > stratetgy instead, chances of success are very unlikely. > > This adds an AVOption to respect that header. > > Signed-off-by: Derek Buitenhuis <derek.buitenhuis@gmail.com> > --- > libavformat/http.c | 12 ++++++++++++ > libavformat/version.h | 2 +- > 2 files changed, 13 insertions(+), 1 deletion(-) Is this feature standardized in a RFC, or is it some other spec somewhere? I think it would be nice with a link to a spec in the commit message here. > > diff --git a/libavformat/http.c b/libavformat/http.c > index e7603037f4..5ed481b63a 100644 > --- a/libavformat/http.c > +++ b/libavformat/http.c > @@ -138,6 +138,8 @@ typedef struct HTTPContext { > char *new_location; > AVDictionary *redirect_cache; > uint64_t filesize_from_content_range; > + int respect_retry_after; > + unsigned int retry_after; > } HTTPContext; > > #define OFFSET(x) offsetof(HTTPContext, x) > @@ -176,6 +178,7 @@ static const AVOption options[] = { > { "reconnect_on_http_error", "list of http status codes to reconnect on", OFFSET(reconnect_on_http_error), AV_OPT_TYPE_STRING, { .str = NULL }, 0, 0, D }, > { "reconnect_streamed", "auto reconnect streamed / non seekable streams", OFFSET(reconnect_streamed), AV_OPT_TYPE_BOOL, { .i64 = 0 }, 0, 1, D }, > { "reconnect_delay_max", "max reconnect delay in seconds after which to give up", OFFSET(reconnect_delay_max), AV_OPT_TYPE_INT, { .i64 = 120 }, 0, UINT_MAX/1000/1000, D }, > + { "respect_retry_after", "respect the Retry-After header when retrying connections", OFFSET(respect_retry_after), AV_OPT_TYPE_BOOL, { .i64 = 1 }, 0, 1, D }, > { "listen", "listen on HTTP", OFFSET(listen), AV_OPT_TYPE_INT, { .i64 = 0 }, 0, 2, D | E }, > { "resource", "The resource requested by a client", OFFSET(resource), AV_OPT_TYPE_STRING, { .str = NULL }, 0, 0, E }, > { "reply_code", "The http status code to return to a client", OFFSET(reply_code), AV_OPT_TYPE_INT, { .i64 = 200}, INT_MIN, 599, E}, > @@ -386,6 +389,13 @@ redo: > reconnect_delay > s->reconnect_delay_max) > goto fail; > > + if (s->respect_retry_after && s->retry_after > 0) { > + reconnect_delay = s->retry_after; It'd be nice with a comment to clarify the units of both values here, which apparently both happen to be integer seconds? > + if (reconnect_delay > s->reconnect_delay_max) > + goto fail; > + s->retry_after = 0; > + } > + > av_log(h, AV_LOG_WARNING, "Will reconnect at %"PRIu64" in %d second(s).\n", off, reconnect_delay); > ret = ff_network_sleep_interruptible(1000U * 1000 * reconnect_delay, &h->interrupt_callback); > if (ret != AVERROR(ETIMEDOUT)) > @@ -1231,6 +1241,8 @@ static int process_line(URLContext *h, char *line, int line_count, int *parsed_h > parse_expires(s, p); > } else if (!av_strcasecmp(tag, "Cache-Control")) { > parse_cache_control(s, p); > + } else if (!av_strcasecmp(tag, "Retry-After")) { > + s->retry_after = strtoul(p, NULL, 10); Can you add a comment here, to clarify what unit the value is expressed in? // Martin
On 4/24/2024 12:06 PM, Martin Storsjö wrote: > Is this feature standardized in a RFC, or is it some other spec somewhere? > I think it would be nice with a link to a spec in the commit message here. It is in the RFC for 429 I noted in the commit I added that: RFC6585. It is also probably in the 503 and 301 RFCs. See also: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Retry-After I will add those. >> + if (s->respect_retry_after && s->retry_after > 0) { >> + reconnect_delay = s->retry_after; > > It'd be nice with a comment to clarify the units of both values here, > which apparently both happen to be integer seconds? Yes, seconds. I have added: /* Both the Retry-After header and the option are in seconds. */ >> + } else if (!av_strcasecmp(tag, "Retry-After")) { >> + s->retry_after = strtoul(p, NULL, 10); > > Can you add a comment here, to clarify what unit the value is expressed > in? Added: /* Specifies how long to wait before retrying in second. */ - Derek
diff --git a/libavformat/http.c b/libavformat/http.c index e7603037f4..5ed481b63a 100644 --- a/libavformat/http.c +++ b/libavformat/http.c @@ -138,6 +138,8 @@ typedef struct HTTPContext { char *new_location; AVDictionary *redirect_cache; uint64_t filesize_from_content_range; + int respect_retry_after; + unsigned int retry_after; } HTTPContext; #define OFFSET(x) offsetof(HTTPContext, x) @@ -176,6 +178,7 @@ static const AVOption options[] = { { "reconnect_on_http_error", "list of http status codes to reconnect on", OFFSET(reconnect_on_http_error), AV_OPT_TYPE_STRING, { .str = NULL }, 0, 0, D }, { "reconnect_streamed", "auto reconnect streamed / non seekable streams", OFFSET(reconnect_streamed), AV_OPT_TYPE_BOOL, { .i64 = 0 }, 0, 1, D }, { "reconnect_delay_max", "max reconnect delay in seconds after which to give up", OFFSET(reconnect_delay_max), AV_OPT_TYPE_INT, { .i64 = 120 }, 0, UINT_MAX/1000/1000, D }, + { "respect_retry_after", "respect the Retry-After header when retrying connections", OFFSET(respect_retry_after), AV_OPT_TYPE_BOOL, { .i64 = 1 }, 0, 1, D }, { "listen", "listen on HTTP", OFFSET(listen), AV_OPT_TYPE_INT, { .i64 = 0 }, 0, 2, D | E }, { "resource", "The resource requested by a client", OFFSET(resource), AV_OPT_TYPE_STRING, { .str = NULL }, 0, 0, E }, { "reply_code", "The http status code to return to a client", OFFSET(reply_code), AV_OPT_TYPE_INT, { .i64 = 200}, INT_MIN, 599, E}, @@ -386,6 +389,13 @@ redo: reconnect_delay > s->reconnect_delay_max) goto fail; + if (s->respect_retry_after && s->retry_after > 0) { + reconnect_delay = s->retry_after; + if (reconnect_delay > s->reconnect_delay_max) + goto fail; + s->retry_after = 0; + } + av_log(h, AV_LOG_WARNING, "Will reconnect at %"PRIu64" in %d second(s).\n", off, reconnect_delay); ret = ff_network_sleep_interruptible(1000U * 1000 * reconnect_delay, &h->interrupt_callback); if (ret != AVERROR(ETIMEDOUT)) @@ -1231,6 +1241,8 @@ static int process_line(URLContext *h, char *line, int line_count, int *parsed_h parse_expires(s, p); } else if (!av_strcasecmp(tag, "Cache-Control")) { parse_cache_control(s, p); + } else if (!av_strcasecmp(tag, "Retry-After")) { + s->retry_after = strtoul(p, NULL, 10); } } return 1; diff --git a/libavformat/version.h b/libavformat/version.h index 7ff1483912..ee91990360 100644 --- a/libavformat/version.h +++ b/libavformat/version.h @@ -32,7 +32,7 @@ #include "version_major.h" #define LIBAVFORMAT_VERSION_MINOR 3 -#define LIBAVFORMAT_VERSION_MICRO 100 +#define LIBAVFORMAT_VERSION_MICRO 101 #define LIBAVFORMAT_VERSION_INT AV_VERSION_INT(LIBAVFORMAT_VERSION_MAJOR, \ LIBAVFORMAT_VERSION_MINOR, \
429 and 503 codes can, and often do (e.g. all Google Cloud Storage URLs can), return a Retry-After header with the error, indicating how long to wait, in seconds, before retrying again. If it is not respected by, for example, using our default backoff stratetgy instead, chances of success are very unlikely. This adds an AVOption to respect that header. Signed-off-by: Derek Buitenhuis <derek.buitenhuis@gmail.com> --- libavformat/http.c | 12 ++++++++++++ libavformat/version.h | 2 +- 2 files changed, 13 insertions(+), 1 deletion(-)