Re: [gupnp] handling of sequence numbers in gupnp-service-proxy.c

[Sven Neumann <s neumann phase-zero de>]

Hi,

On Thu, 2009-03-26 at 15:52 +0200, Zeeshan Ali (Khattak) wrote:

> > I understood that the libsoup developer confirmed this on the basis that
> > all messages are sent on the same HTTP connection. Is this a valid
> > assumption to be made for GUPnPService notification events ?
> 
>    Yes it is since each GUPnPContext and therefore each GUPnPService
> is supposed to be bound to only and only one network interface. Good
> that you mention this as i noticed a bug in gupnp-context.c: It does
> not specify the address/interface when creating the SoupServer and
> hence the server created is not bound to a particular interface as it
> should be. I'll fix that as part of my multiple network support
> adventure when i get back to that.

OK, the part about specifying the network interface is a separate
problem. I'll let you deal with that.

But after talking to Dan Winship we found that we can't assume that
libsoup will use a single connection unless we explicitly tell it so
using the SOUP_SESSION_MAX_CONNS_PER_HOST option. If I set that option
in the test case, then the assertion is never hit.

Attached are two patches. One changes GUPnPServiceProxy to not discard
the message (that is the same patch I sent earlier). The second patch
introduces a dedicated SoupSession in GUPnPService. This dedicated
session uses a single HTTP connection and is used for notification
events.

I'll also open a bug report for this.


Sven


>From c68a07fcad7a7a9b928d8c61d47adbc3ca3762eb Mon Sep 17 00:00:00 2001
From: Sven Neumann <s neumann phase-zero de>
Date: Thu, 26 Mar 2009 13:58:12 +0100
Subject: [PATCH] don't discard the message when receiving an unexpected sequence number

---
 libgupnp/gupnp-service-proxy.c |    5 -----
 1 files changed, 0 insertions(+), 5 deletions(-)

diff --git a/libgupnp/gupnp-service-proxy.c b/libgupnp/gupnp-service-proxy.c
index 248cc25..b1b8ea9 100644
--- a/libgupnp/gupnp-service-proxy.c
+++ b/libgupnp/gupnp-service-proxy.c
@@ -1492,11 +1492,6 @@ server_handler (SoupServer        *soup_server,
                 /* Oops, we missed a notify. Resubscribe .. */
                 unsubscribe (proxy);
                 subscribe (proxy);
-
-                /* Message was OK otherwise */
-                soup_message_set_status (msg, SOUP_STATUS_OK);
-
-                return;
         }
 
         /* Increment our own event sequence number */
-- 
1.5.6.3

>From 65292d451be2029e540c85e88e107ca0f55c42c3 Mon Sep 17 00:00:00 2001
From: Sven Neumann <s neumann phase-zero de>
Date: Thu, 26 Mar 2009 16:33:55 +0100
Subject: [PATCH] use a dedicated SoupSession with MAX_CONNECTIONS 1 for GUPnPService

---
 libgupnp/gupnp-service.c |   63 ++++++++++++++++++++++++++++++++-------------
 1 files changed, 45 insertions(+), 18 deletions(-)

diff --git a/libgupnp/gupnp-service.c b/libgupnp/gupnp-service.c
index e7c8013..61c6c9a 100644
--- a/libgupnp/gupnp-service.c
+++ b/libgupnp/gupnp-service.c
@@ -49,6 +49,8 @@ G_DEFINE_TYPE (GUPnPService,
 struct _GUPnPServicePrivate {
         GUPnPRootDevice *root_device;
 
+        SoupSession     *session;
+
         guint            notify_available_id;
 
         GHashTable      *subscriptions;
@@ -96,15 +98,41 @@ typedef struct {
                                            subscription */
 } SubscriptionData;
 
+static SoupSession *
+gupnp_service_get_session (GUPnPService *service)
+{
+        if (! service->priv->session) {
+                GUPnPContext *context =
+                  gupnp_service_info_get_context (GUPNP_SERVICE_INFO (service));
+
+                /* Create a dedicated session for this service to
+                 * ensure that notifications are sent in the proper
+                 * order. The session from GUPnPContext may use
+                 * multiple connections.
+                 */
+                service->priv->session = soup_session_async_new_with_options
+                  (SOUP_SESSION_IDLE_TIMEOUT, 60,
+                   SOUP_SESSION_ASYNC_CONTEXT,
+                   gssdp_client_get_main_context (GSSDP_CLIENT (context)),
+                   SOUP_SESSION_MAX_CONNS_PER_HOST, 1,
+                   NULL);
+
+                if (g_getenv ("GUPNP_DEBUG")) {
+                        SoupLogger *logger;
+                        logger = soup_logger_new (SOUP_LOGGER_LOG_BODY, -1);
+                        soup_logger_attach (logger, service->priv->session);
+                }
+        }
+
+        return service->priv->session;
+}
+
 static void
 subscription_data_free (SubscriptionData *data)
 {
-        GUPnPContext *context;
         SoupSession *session;
 
-        context = gupnp_service_info_get_context
-                        (GUPNP_SERVICE_INFO (data->service));
-        session = gupnp_context_get_session (context);
+        session = gupnp_service_get_session (data->service);
 
         /* Cancel pending messages */
         while (data->pending_messages) {
@@ -559,19 +587,19 @@ gupnp_service_action_return_error (GUPnPServiceAction *action,
 }
 
 static void
-gupnp_service_init (GUPnPService *device)
+gupnp_service_init (GUPnPService *service)
 {
-        device->priv = G_TYPE_INSTANCE_GET_PRIVATE (device,
-                                                    GUPNP_TYPE_SERVICE,
-                                                    GUPnPServicePrivate);
+        service->priv = G_TYPE_INSTANCE_GET_PRIVATE (service,
+                                                     GUPNP_TYPE_SERVICE,
+                                                     GUPnPServicePrivate);
 
-        device->priv->subscriptions =
+        service->priv->subscriptions =
                 g_hash_table_new_full (g_str_hash,
                                        g_str_equal,
                                        NULL,
                                        (GDestroyNotify) subscription_data_free);
 
-        device->priv->notify_queue = g_queue_new ();
+        service->priv->notify_queue = g_queue_new ();
 }
 
 /* Generate a new action response node for @action_name */
@@ -1310,6 +1338,11 @@ gupnp_service_finalize (GObject *object)
 
         g_queue_free (service->priv->notify_queue);
 
+        if (service->priv->session) {
+                g_object_unref (service->priv->session);
+                service->priv->session = NULL;
+        }
+
         /* Call super */
         object_class = G_OBJECT_CLASS (gupnp_service_parent_class);
         object_class->finalize (object);
@@ -1516,7 +1549,6 @@ notify_got_response (SoupSession *session,
                 /* Other failure: Try next callback or signal failure. */
                 if (data->callbacks->next) {
                         SoupURI *uri;
-                        GUPnPContext *context;
                         SoupSession *session;
 
                         /* Call next callback */
@@ -1530,9 +1562,7 @@ notify_got_response (SoupSession *session,
                         data->pending_messages = 
                                 g_list_prepend (data->pending_messages, msg);
 
-                        context = gupnp_service_info_get_context
-                                        (GUPNP_SERVICE_INFO (data->service));
-                        session = gupnp_context_get_session (context);
+                        session = gupnp_service_get_session (data->service);
 
                         soup_session_requeue_message (session, msg);
                 } else {
@@ -1568,7 +1598,6 @@ notify_subscriber (gpointer key,
         const char *property_set;
         char *tmp;
         SoupMessage *msg;
-        GUPnPContext *context;
         SoupSession *session;
 
         data = value;
@@ -1617,9 +1646,7 @@ notify_subscriber (gpointer key,
         /* Queue */
         data->pending_messages = g_list_prepend (data->pending_messages, msg);
 
-        context = gupnp_service_info_get_context
-                        (GUPNP_SERVICE_INFO (data->service));
-        session = gupnp_context_get_session (context);
+        session = gupnp_service_get_session (data->service);
 
         soup_session_queue_message (session,
                                     msg,
-- 
1.5.6.3