[FFmpeg-devel] [PATCH] http: support retry on connection error

[Eran Kornblau]

Hi Marton,

Thank you for the feedback, and sorry for my late reply :)
Please see my responses inline, updated patch attached.

Thanks
Eran

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of Marton Balint
> Sent: Wednesday, November 18, 2020 10:46 PM
> To: FFmpeg development discussions and patches <ffmpeg-devel at ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH] http: support retry on connection error
> 
> 
> > - reconnect_on_status - a list of http status codes that should be
> >     retried. the list can contain explicit status codes / the strings
> >     4xx/5xx.
> 
> Maybe better name this option reconnect_on_http_error. Also this is a flags-like option, so use AV_OPT_TYPE_FLAGS with 4xx and 5xx as flags.
> 
Rename - done.

Flags - the patch supports both explicit status codes (e.g. 503) and status groups (e.g. 4xx).
For example, it is possible to specify -reconnect_on_http_error '4xx,503'.

In my understanding, AV_OPT_TYPE_FLAGS is a simple int, so the only to support what I wrote above, is to assign bits
to specific status codes, and I don't think we want to go there... (because every time someone will want to retry on a new status code,
we will need to update the code)

> 
> > - reconnect_on_err - reconnects on arbitrary errors during connect, e.g.
> >     ECONNRESET/ETIMEDOUT
> 
> Maybe reconnect_on_network_error better reflects that this reconnects on 
> underlying protocol (TCP/TLS) error.
> 
> Anyhow, the new options should be added to docs/protocols.texi.
> 
Rename & docs - done.

> > 
> > +    { "reconnect_on_err", "auto reconnect in case of tcp/tls error during connect", OFFSET(reconnect_on_err), AV_OPT_TYPE_BOOL, { .i64 = 0 }, 0, 1, D },
> > +    { "reconnect_on_status", "list of http status codes to reconnect on. the list can include specific status codes / 4xx / 5xx", OFFSET(reconnect_on_status), AV_OPT_TYPE_STRING, { .str = NULL }, 0, 0, D },
> 
> AV_OPT_TYPE_FLAGS, as I explained above.
> 
Replied above

> > 
> >      location_changed = http_open_cnx_internal(h, options);
> 
> Are you sure you get AVERROR_HTTP_* here? Also if you follow the code 
> there is special handling of 401/407, that should be done first before 
> your checks, no?
> 
1. Yes, I tested the change before submitting, used some test PHP server to simulate errors.
    This is the call stack that propagates the AVERROR_HTTP_* codes -
    http_open_cnx_internal
    http_connect
    http_read_header
    process_line
    check_http_code

2. In case of 401/407, check_http_code returns 0, and therefore http_open_cnx_internal return 0 as well (assuming there is no other error...). 
    So, it doesn't enter 'if (location_changed < 0)' and it behaves the same way it did before my changes.

> 
> Another comment is that if you want to reconnect based on errors 
> classified as 4xx or 5xx, then probably you should revisit 
> ff_http_averror(400, xxx) calls and check if they are indeed caused by 
> client behaviour. Because if the server is violating the protocol, then 
> they should be ff_http_averror(500, xxx) IMHO.
> 
Checked the code, I see 2 cases of ff_http_averror(400, xxx) -
1. HTTP server received an unexpected http method (would have been more correct to use 405, but don't think it matters...)
2. HTTP server received an invalid http version string
So, I think it's fine.


> Regards,
> Marton
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://eur02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fffmpeg.org%2Fmailman%2Flistinfo%2Fffmpeg-devel&data=04%7C01%7Ceran.kornblau%40kaltura.com%7C590db1ef6c4d484bb34508d88c030453%7C0c503748de3f4e2597e26819d53a42b6%7C0%7C0%7C637413291875784243%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=hemm499pmQQQEDOc4FXeq1EThvOT7FyfjWRsylFvdm0%3D&reserved=0
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-http-support-retry-on-connection-error.patch
Type: application/octet-stream
Size: 5546 bytes
Desc: 0001-http-support-retry-on-connection-error.patch
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20201203/5919e103/attachment.obj>