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

[Eran Kornblau]

Ping...

-----Original Message-----
From: Eran Kornblau 
Sent: Thursday, December 10, 2020 8:25 AM
To: FFmpeg development discussions and patches <ffmpeg-devel at ffmpeg.org>
Subject: RE: [FFmpeg-devel] [PATCH] http: support retry on connection error

Resending...

-----Original Message-----
From: Eran Kornblau
Sent: Thursday, December 3, 2020 10:52 AM
To: FFmpeg development discussions and patches <ffmpeg-devel at ffmpeg.org>
Subject: RE: [FFmpeg-devel] [PATCH] http: support retry on connection error

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%2Fffmp
> eg.org%2Fmailman%2Flistinfo%2Fffmpeg-devel&data=04%7C01%7Ceran.kor
> nblau%40kaltura.com%7C590db1ef6c4d484bb34508d88c030453%7C0c503748de3f4
> e2597e26819d53a42b6%7C0%7C0%7C637413291875784243%7CUnknown%7CTWFpbGZsb
> 3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%
> 7C1000&sdata=hemm499pmQQQEDOc4FXeq1EThvOT7FyfjWRsylFvdm0%3D&re
> served=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/20201216/b9e0263b/attachment.obj>