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

[Marton Balint]

On Wed, 30 Dec 2020, Eran Kornblau wrote:

> It's been 2 months since I submitted this patch (25/10), got a single reply to which I answered below... Can this be merged?

I will apply in a few days if no futher comments are received.

Thanks,
Marton

>
> Thanks,
>
> Eran
>
> -----Original Message-----
> From: Eran Kornblau
> Sent: Wednesday, December 23, 2020 8:14 PM
> To: FFmpeg development discussions and patches <ffmpeg-devel at ffmpeg.org>
> Subject: FW: [FFmpeg-devel] [PATCH] http: support retry on connection error
>
> Pinging again... hope this can be merged...
>
> -----Original Message-----
> From: Eran Kornblau
> Sent: Wednesday, December 16, 2020 9:18 PM
> To: FFmpeg development discussions and patches <ffmpeg-devel at ffmpeg.org>
> Subject: FW: [FFmpeg-devel] [PATCH] http: support retry on connection error
>
> 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".
>>
>