GNOME Bugzilla – Bug 756318
souphttpclientsink: Add "retry" for request failures
Last modified: 2015-10-14 12:23:41 UTC
Created attachment 312985 [details] [review] Add the retry option to allow a failed request to be retried after the given number of seconds instead of failing the pipeline. Take account of the Retry-After header if present. Currently souphttpclientsink terminates the whole pipeline on any error, which breaks streaming applications where streams are long lived and must recover from temporary network glitches. Add support for the "retry" option, which gives the number of seconds to back off and wait before retrying the request again. The also adds support for honouring the Retry-After header when present.
Review of attachment 312985 [details] [review]: ::: ext/soup/gstsouphttpclientsink.c @@ +176,3 @@ G_TYPE_STRV, G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS)); + g_object_class_install_property (gobject_class, PROP_RETRY, + g_param_spec_int ("retry", "Retry", retry-delay might be better @@ +753,2 @@ if (!SOUP_STATUS_IS_SUCCESSFUL (msg->status_code)) { + if (souphttpsink->retry > 0) { Why only look at the header if the property was set? Shouldn't we always look at the header and respect it? (Also is this also relevant for souphttpsrc?) @@ +758,3 @@ + "Retry-After"); + if (retry_after) { + retry = atoi (retry_after); g_ascii_strtoull() might be better here @@ +760,3 @@ + retry = atoi (retry_after); + if (retry <= 0) { + retry = souphttpsink->retry; Should this just be a MAX(retry, souphttpsink->retry)? @@ +771,3 @@ + msg->reason_phrase, retry); + } + g_usleep (G_USEC_PER_SEC * retry); Don't use g_usleep() like that, it's not interruptible. If nothing else is available for asynchronous waiting here, use a GCond and make sure to unlock it when needed
> retry-delay might be better Agreed. > Why only look at the header if the property was set? Shouldn't we > always look at the header and respect it? The property defaults to the current behaviour, which is to fail the pipeline completely. If we changes this we would break existing configurations. > g_ascii_strtoull() might be better here Will change this. > Should this just be a MAX(retry, souphttpsink->retry)? Depends on whose config should win, the client or the server. I guess the server config should win, I'll change this. > Don't use g_usleep() like that, it's not interruptible. If nothing > else is available for asynchronous waiting here, use a GCond and make > sure to unlock it when needed Do you have an example I can look at of how to implement an interruptible timer safely? I searched the existing codebase of examples and only found the use of g_usleep().
Created attachment 313089 [details] [review] Add the retry option to allow a failed request to be retried after the given number of seconds instead of failing the pipeline. Take account of the Retry-After header if present. Updated patch.
(In reply to minfrin from comment #2) > > Why only look at the header if the property was set? Shouldn't we > > always look at the header and respect it? > > The property defaults to the current behaviour, which is to fail the > pipeline completely. If we changes this we would break existing > configurations. Maybe there should be retry-delay and a retries property. The latter just defining how many times it should retry after it fails completely. > > Should this just be a MAX(retry, souphttpsink->retry)? > > Depends on whose config should win, the client or the server. I guess the > server config should win, I'll change this. ACK > > Don't use g_usleep() like that, it's not interruptible. If nothing > > else is available for asynchronous waiting here, use a GCond and make > > sure to unlock it when needed > > Do you have an example I can look at of how to implement an interruptible > timer safely? I searched the existing codebase of examples and only found > the use of g_usleep(). Use a GCond and GMutex here, and g_cond_wait_until()
Created attachment 313139 [details] [review] Rename retry to retry-after, add retries parameter that controls the number of times an HTTP request will be retried before failing. Additional patch that adds the retries option that controls the number of times a retry will be attempted. This patch also implements the interruptible wait.
This has been updated for git-master.
Review of attachment 313139 [details] [review]: ::: ext/soup/gstsouphttpclientsink.c @@ +790,2 @@ if (!SOUP_STATUS_IS_SUCCESSFUL (msg->status_code)) { + souphttpsink->failures++; Wouldn't this only retry once here then? I think you need a loop around all this code for the number of retries @@ +823,3 @@ + end_time); + while (res == TRUE && g_main_loop_is_running (souphttpsink->loop)); + if (g_main_loop_is_running (souphttpsink->loop)) { As we have a mainloop here, you could also wait on that with a g_timeout_source_new() probably
> Wouldn't this only retry once here then? I think you need a loop > around all this code for the number of retries We use the asynchronous libsoup code, and as a result requests are scheduled rather than executed synchronously. You retry by rescheduling the same request again and leaving. You decide to fail by writing the result code away and leaving, this gets propagated to the gstreamer thread which then decides to shut down (or not). There is no looping in the async code.
Right, then you can easily do the waiting asynchronously too with g_timeout_source_new() instead of GCond :)
Created attachment 313172 [details] [review] Switch to the use of g_timeout_source_new() instead of GCond Third time's a charm.
Comment on attachment 313172 [details] [review] Switch to the use of g_timeout_source_new() instead of GCond Seems good but can you squash all patches together into one? :)
I went on a google mission to figure out how to convince git format-patch to do that, and wasn't able to find any clear instructions. Is there a set of options that will do that?
You can use "git rebase --interactive" for that, that allows you to rewrite history and squash together commits, reorder them, drop them, etc.
Created attachment 313175 [details] [review] Add the retry option to allow a failed request to be retried after the given number of seconds instead of failing the pipeline. Take account of the Retry-After header if present. Add retries paramet Add the combined patch after a rewrite of history.
Created attachment 313186 [details] [review] souphttpclientsink: Add the retry and retry-delay properties These allow a failed request to be retried after the given number of seconds instead of failing the pipeline. Take account of the Retry-After header if present. Add retries parameter that controls the number of times an HTTP request will be retried before failing.
Review of attachment 313186 [details] [review]: Did some minor changes to your patch, like the commit message. But: ::: ext/soup/gstsouphttpclientsink.c @@ +180,3 @@ + g_object_class_install_property (gobject_class, PROP_RETRIES, + g_param_spec_int ("retries", "Retries", + "Maximum number of retries, set to zero to keep trying", What about -1 to retry forever, 0 to never retry, 1 to retry once, etc. IMHO should be independent of retry-delay property. Setting retry-delay=0 should just make it retry immediately.
> What about -1 to retry forever, 0 to never retry, 1 to retry once, > etc. IMHO should be independent of retry-delay property. Setting > retry-delay=0 should just make it retry immediately. "Retry immediately" is a Denial of Service on the server, the most destructive case being "retry immediately forever". We shouldn't expose this option at all.
Retry immediately 2-3 times should be ok, or maybe make the retry-delay in milliseconds and allow it to be set to 100ms at minimum.
How about this: we drive it from retries, -1 means forever, zero means don't retry, more than one means retry that many times. Retry-after sets the number of seconds and defaults to 5. Patch to follow.
Created attachment 313192 [details] [review] Add the retry option to allow a failed request to be retried after the given number of seconds instead of failing the pipeline. Take account of the Retry-After header if present. Add retries paramet
I attached an updated patch based on yours here because of the commit message and some minor other changes, would've been good if you took that instead ;) 5s seems a lot of time, it will most likely mean that any live'ish streaming to a server will be completely off afterwards... even if the problem was just a hickup at the server.
Didn't see your patch at all until my patch was done, sorry. What did you change? The reason for the "backing off" is to ensure that when the network is in a congested or temporarily degraded state, we don't completely overwhelm that network and make it worse. 5 seconds isn't unreasonable in this case, and the server will catch up quickly when the network resumes.
Some minor cleanup, I remember the commit message, the dropping of #include <stdlib.h> and using GST_WARNING_OBJECT instead of GST_WARNING.
Created attachment 313198 [details] [review] souphttpclientsink: Add the retry and retry-delay properties Updated patch with unused imports removed, GST_WARN becomes GST_WARN_OBJECT.
Something like this would probably also be useful for souphttpsrc :) commit af25e3cc93ec5c6d5fce8e0b9631a76423134a02 Author: Graham Leggett <minfrin@sharp.fm> Date: Sun Oct 11 22:07:54 2015 +0000 souphttpclientsink: Add the retry and retry-delay properties These allow a failed request to be retried after the given number of seconds instead of failing the pipeline. Take account of the Retry-After header if present. Add retries parameter that controls the number of times an HTTP request will be retried before failing. https://bugzilla.gnome.org/show_bug.cgi?id=756318