Resending... -----Original Message----- From: Eran Kornblau Sent: Thursday, December 3, 2020 10:52 AM To: FFmpeg development discussions and patches <ffmpeg-devel@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-boun...@ffmpeg.org> On Behalf Of
> Marton Balint
> Sent: Wednesday, November 18, 2020 10:46 PM
> To: FFmpeg development discussions and patches
> <ffmpeg-devel@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@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-requ...@ffmpeg.org with subject "unsubscribe".
>
0001-http-support-retry-on-connection-error.patch
Description: 0001-http-support-retry-on-connection-error.patch
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
