GNOME Bugzilla – Bug 694450
Control point doesn't retry to get the description document
Last modified: 2016-06-18 15:17:38 UTC
We have come upon a weird situation when we get ssdp notifications from a device that just became available, but for a short time http(tcp) conections to that device fail, ie GUPnPControlPoint reports: (null)-WARNING: Failed to GET http://192.168.1.157:40166/00000000-0000-0000-0000-001966374ef6.xml: Cannot connect to destination (192.168.1.157) This means that GUPnPControlPoint receives the resource_available notification, but is unable to retrieve the description url at that time. After such failure, the control point gives up, doesn't try to retrieve the description anymore and is able to properly annouce the device/service only after GSSDP reports it unavailable and then available again (ie after the device restarts). Steps to Reproduce 1. Have a device that is unable to receive / respond to tcp connections for a short time after it is turned on / starts to announce itself (it is possible to reproduce simply this by dropping all tcp traffic to/from the device when it starts) Actual Results GUPnPControlPoint fails to recognize the device/service until it is restarted, even if (tcp) connectivity to the device is restored and ssdp alive packets keep comming in. Expected Results GUPnPControlPoint should recognize the device/service as soon as it becomes possible to download the description or when the next ssdp alive notification comes The attached patch fixes this by retrying to fetch the description until it either succeeds or resource_unavailable for that device/service is received. Not sure if this is the best thing to do. Maybe only retrying when the next ssdp alive comes would be best, but I don't know how to get such information from GSSDPResourceBrowser.
Created attachment 237182 [details] [review] Retry getting description url in control point
Created attachment 237187 [details] [review] 237182: Retry getting description url in control point Fixed patch, I missed a line in the previous one.
Review of attachment 237187 [details] [review]: Looks good in general. If I read the code correctly, this keeps on retrying to download the document forever? Wouldn't it be better to give up after some tries/time? ::: libgupnp/gupnp-control-point.c @@ +914,3 @@ + if (get_data) { + get_description_url_data_free (get_data); + } No curlies
Review of attachment 237187 [details] [review]: Huh, never noticed this bug before... The retry method that has worked for me in other areas is starting with a timeout value and doubling that on failure (stopping at some point still sounds reasonable) -- this way it's OK to start with a small value so the "normal" case of short non-connectivity works as well as possible. ::: libgupnp/gupnp-control-point.c @@ +126,3 @@ + if ((strcmp(udn, data->udn) == 0) && + (service_type == data->service_type) && + GList *l = control_point->priv->pending_gets; Doesn't this go wrong when service_type is non-NULL?
(In reply to comment #4) > Doesn't this go wrong when service_type is non-NULL? Er, that was supposed to refer to this: + if ((strcmp(udn, data->udn) == 0) && + (service_type == data->service_type) && + (!service_type || (strcmp(service_type, data->service_type) == 0)))
Review of attachment 237187 [details] [review]: ::: libgupnp/gupnp-control-point.c @@ +126,3 @@ + if ((strcmp(udn, data->udn) == 0) && + (service_type == data->service_type) && + (!service_type || (strcmp(service_type, data->service_type) == 0))) Yeah, the boolean is a bit odd here, I think... Somethink like (service_type == data->service_type || (service_type && (strcmp(service_type, data->service_type) == 0))) ?
or even (service_type == data->service_type || g_strcmp0 (service_type, data->service_type) == 0)
Created attachment 277458 [details] [review] control-point: retry get description url on failure Retry 4 times, doubling the intial timeout of 5s https://bugzilla.gnome.org/show_bug.cgi?id=694450 Signed-off-by: Jens Georg <mail@jensge.org>
Comment on attachment 277458 [details] [review] control-point: retry get description url on failure Mh, no, that doesn't work
I love how I documented WHY my last patch does not work -.-
Created attachment 329994 [details] [review] control-point: retry get description url on failure Retry 4 times, doubling the intial timeout of 5s https://bugzilla.gnome.org/show_bug.cgi?id=694450 Signed-off-by: Jens Georg <mail@jensge.org>
Found out what was wrong with the previous patch. The increased timeout and decreased try count wasn't propagated to the next round
Attachment 329994 [details] pushed as cc4550f - control-point: retry get description url on failure