Make SoupConnection warn if disposed "incorrectly"

Connections should always be disconnected and message-less by the time
they are disposed. Likewise, their SoupSockets should be disconnected
and not in the middle of an async connect when they're freed (though
we can't make the sockets warn unconditionally, since other SoupSocket
users external to libsoup may still be doing it "wrong", since that has
historically been a supported part of the API)

Fix some problems revealed by these warnings. Also, remove some weak
refs and replace others by hard refs as needed. Remove unnecessary
SoupSessionAsyncTunnelData type.

diff --git a/libsoup/soup-connection.c b/libsoup/soup-connection.c
index d1cbfca..98e2730 100644 (file)
--- a/libsoup/soup-connection.c
+++ b/libsoup/soup-connection.c
@@ -119,8 +119,14 @@ dispose (GObject *object)
        /* Make sure clear_current_request doesn't re-establish the timeout */
        priv->idle_timeout = 0;
 
-       clear_current_request (conn);
-       soup_connection_disconnect (conn);
+       if (priv->cur_req) {
+               g_warning ("Disposing connection with cur_req set");
+               clear_current_request (conn);
+       }
+       if (priv->socket) {
+               g_warning ("Disposing connection while connected");
+               soup_connection_disconnect (conn);
+       }
 
        G_OBJECT_CLASS (soup_connection_parent_class)->dispose (object);
 }
@@ -369,8 +375,6 @@ set_current_request (SoupConnection *conn, SoupMessage *req)
            req->method != SOUP_METHOD_CONNECT)
                soup_connection_set_state (conn, SOUP_CONNECTION_IN_USE);
 
-       g_object_add_weak_pointer (G_OBJECT (req), (gpointer)&priv->cur_req);
-
        g_object_thaw_notify (G_OBJECT (conn));
 }
 
@@ -394,8 +398,6 @@ clear_current_request (SoupConnection *conn)
        if (priv->cur_req) {
                SoupMessage *cur_req = priv->cur_req;
 
-               g_object_remove_weak_pointer (G_OBJECT (priv->cur_req),
-                                             (gpointer)&priv->cur_req);
                priv->cur_req = NULL;
                g_object_notify (G_OBJECT (conn), "message");
 
@@ -426,18 +428,6 @@ socket_connect_result (SoupSocket *sock, guint status, gpointer user_data)
        SoupConnectionAsyncConnectData *data = user_data;
        SoupConnectionPrivate *priv;
 
-       if (!data->conn) {
-               if (data->callback) {
-                       data->callback (NULL, SOUP_STATUS_CANCELLED,
-                                       data->callback_data);
-               }
-               g_slice_free (SoupConnectionAsyncConnectData, data);
-               return;
-       }
-
-       g_object_remove_weak_pointer (G_OBJECT (data->conn),
-                                     (gpointer *)&data->conn);
-
        priv = SOUP_CONNECTION_GET_PRIVATE (data->conn);
 
        if (!SOUP_STATUS_IS_SUCCESSFUL (status))
@@ -463,6 +453,7 @@ socket_connect_result (SoupSocket *sock, guint status, gpointer user_data)
                        status = soup_status_proxify (status);
                data->callback (data->conn, status, data->callback_data);
        }
+       g_object_unref (data->conn);
        g_slice_free (SoupConnectionAsyncConnectData, data);
 }
 
@@ -489,10 +480,9 @@ soup_connection_connect_async (SoupConnection *conn,
        soup_connection_set_state (conn, SOUP_CONNECTION_CONNECTING);
 
        data = g_slice_new (SoupConnectionAsyncConnectData);
-       data->conn = conn;
+       data->conn = g_object_ref (conn);
        data->callback = callback;
        data->callback_data = user_data;
-       g_object_add_weak_pointer (G_OBJECT (conn), (gpointer *)&data->conn);
 
        priv->socket =
                soup_socket_new (SOUP_SOCKET_REMOTE_ADDRESS, priv->remote_addr,
@@ -500,6 +490,7 @@ soup_connection_connect_async (SoupConnection *conn,
                                 SOUP_SOCKET_SSL_STRICT, priv->ssl_strict,
                                 SOUP_SOCKET_ASYNC_CONTEXT, priv->async_context,
                                 SOUP_SOCKET_TIMEOUT, priv->io_timeout,
+                                "clean-dispose", TRUE,
                                 NULL);
        soup_socket_connect_async (priv->socket, NULL,
                                   socket_connect_result, data);
@@ -531,6 +522,7 @@ soup_connection_connect_sync (SoupConnection *conn)
                                 SOUP_SOCKET_SSL_STRICT, priv->ssl_strict,
                                 SOUP_SOCKET_FLAG_NONBLOCKING, FALSE,
                                 SOUP_SOCKET_TIMEOUT, priv->io_timeout,
+                                "clean-dispose", TRUE,
                                 NULL);
 
        status = soup_socket_connect_sync (priv->socket, NULL);
@@ -676,7 +668,8 @@ soup_connection_set_state (SoupConnection *conn, SoupConnectionState state)
        priv = SOUP_CONNECTION_GET_PRIVATE (conn);
        old_state = priv->state;
        priv->state = state;
-       if (state == SOUP_CONNECTION_IDLE &&
+       if ((state == SOUP_CONNECTION_IDLE ||
+            state == SOUP_CONNECTION_DISCONNECTED) &&
            old_state == SOUP_CONNECTION_IN_USE)
                clear_current_request (conn);
 

diff --git a/libsoup/soup-session-async.c b/libsoup/soup-session-async.c
index b41aab8..c0630ca 100644 (file)
--- a/libsoup/soup-session-async.c
+++ b/libsoup/soup-session-async.c
@@ -210,62 +210,48 @@ connection_closed (SoupConnection *conn, gpointer session)
        do_idle_run_queue (session);
 }
 
-typedef struct {
-       SoupSession *session;
-       SoupConnection *conn;
-       SoupMessageQueueItem *item;
-} SoupSessionAsyncTunnelData;
-
 static void
 tunnel_connected (SoupMessage *msg, gpointer user_data)
 {
-       SoupSessionAsyncTunnelData *data = user_data;
+       SoupMessageQueueItem *item = user_data;
 
        if (!SOUP_STATUS_IS_SUCCESSFUL (msg->status_code)) {
-               soup_session_connection_failed (data->session, data->conn,
+               soup_session_connection_failed (item->session, item->conn,
                                                msg->status_code);
                goto done;
        }
 
-       if (!soup_connection_start_ssl (data->conn)) {
-               soup_session_connection_failed (data->session, data->conn,
+       if (!soup_connection_start_ssl (item->conn)) {
+               soup_session_connection_failed (item->session, item->conn,
                                                SOUP_STATUS_SSL_FAILED);
                goto done;
        }
 
-       g_signal_connect (data->conn, "disconnected",
-                         G_CALLBACK (connection_closed), data->session);
-       soup_connection_set_state (data->conn, SOUP_CONNECTION_IDLE);
+       g_signal_connect (item->conn, "disconnected",
+                         G_CALLBACK (connection_closed), item->session);
+       soup_connection_set_state (item->conn, SOUP_CONNECTION_IDLE);
 
 done:
-       do_idle_run_queue (data->session);
-       soup_message_queue_item_unref (data->item);
-       g_slice_free (SoupSessionAsyncTunnelData, data);
+       do_idle_run_queue (item->session);
+       g_object_unref (item->session);
+       soup_message_queue_item_unref (item);
 }
 
 static void
 tunnel_connect_restarted (SoupMessage *msg, gpointer user_data)
 {
-       SoupSessionAsyncTunnelData *data = user_data;
+       SoupMessageQueueItem *item = user_data;
 
        if (SOUP_MESSAGE_IS_STARTING (msg))
-               soup_session_send_queue_item (data->session, data->item, data->conn);
+               soup_session_send_queue_item (item->session, item, item->conn);
 }
 
 static void
 got_connection (SoupConnection *conn, guint status, gpointer user_data)
 {
-       gpointer *session_p = user_data;
-       SoupSession *session = *session_p;
+       SoupSession *session = user_data;
        SoupAddress *tunnel_addr;
 
-       if (!session) {
-               g_slice_free (gpointer, session_p);
-               return;
-       }
-       g_object_remove_weak_pointer (G_OBJECT (session), session_p);
-       g_slice_free (gpointer, session_p);
-
        if (status != SOUP_STATUS_OK) {
                /* There may have been messages waiting for the
                 * connection count to go down, so queue a run_queue.
@@ -273,25 +259,21 @@ got_connection (SoupConnection *conn, guint status, gpointer user_data)
                do_idle_run_queue (session);
 
                soup_session_connection_failed (session, conn, status);
-               /* session may be destroyed at this point */
-
+               g_object_unref (session);
                return;
        }
 
        tunnel_addr = soup_connection_get_tunnel_addr (conn);
        if (tunnel_addr) {
-               SoupSessionAsyncTunnelData *data;
+               SoupMessageQueueItem *item;
 
-               data = g_slice_new (SoupSessionAsyncTunnelData);
-               data->session = session;
-               data->conn = conn;
-               data->item = soup_session_make_connect_message (session, tunnel_addr);
+               item = soup_session_make_connect_message (session, tunnel_addr);
                g_signal_emit_by_name (session, "tunneling", conn);
-               g_signal_connect (data->item->msg, "finished",
-                                 G_CALLBACK (tunnel_connected), data);
-               g_signal_connect (data->item->msg, "restarted",
-                                 G_CALLBACK (tunnel_connect_restarted), data);
-               soup_session_send_queue_item (session, data->item, conn);
+               g_signal_connect (item->msg, "finished",
+                                 G_CALLBACK (tunnel_connected), item);
+               g_signal_connect (item->msg, "restarted",
+                                 G_CALLBACK (tunnel_connect_restarted), item);
+               soup_session_send_queue_item (session, item, conn);
                return;
        }
 
@@ -310,6 +292,7 @@ got_connection (SoupConnection *conn, guint status, gpointer user_data)
         */
        soup_connection_set_state (conn, SOUP_CONNECTION_IDLE);
        do_idle_run_queue (session);
+       g_object_unref (session);
 }
 
 static void
@@ -353,13 +336,8 @@ run_queue (SoupSessionAsync *sa)
                        continue;
 
                if (soup_connection_get_state (conn) == SOUP_CONNECTION_NEW) {
-                       gpointer *session_p;
-
-                       session_p = g_slice_new (gpointer);
-                       *session_p = session;
-                       g_object_add_weak_pointer (G_OBJECT (session), session_p);
                        soup_connection_connect_async (conn, got_connection,
-                                                      session_p);
+                                                      g_object_ref (session));
                } else
                        soup_session_send_queue_item (session, item, conn);
        }

diff --git a/libsoup/soup-session.c b/libsoup/soup-session.c
index 287d643..55bcf09 100644 (file)
--- a/libsoup/soup-session.c
+++ b/libsoup/soup-session.c
@@ -1274,7 +1274,7 @@ soup_session_connection_failed (SoupSession *session,
        g_mutex_unlock (priv->host_lock);
 
        if (host)
-               connection_disconnected (conn, session);
+               soup_connection_disconnect (conn);
        else if (conn)
                host = g_object_get_data (G_OBJECT (conn), "SoupSessionHost");
        if (!host)

diff --git a/libsoup/soup-socket.c b/libsoup/soup-socket.c
index 9c8fe87..871ec69 100644 (file)
--- a/libsoup/soup-socket.c
+++ b/libsoup/soup-socket.c
@@ -70,6 +70,7 @@ enum {
        PROP_ASYNC_CONTEXT,
        PROP_TIMEOUT,
        PROP_TRUSTED_CERTIFICATE,
+       PROP_CLEAN_DISPOSE,
 
        LAST_PROP
 };
@@ -82,9 +83,10 @@ typedef struct {
        guint non_blocking:1;
        guint is_server:1;
        guint timed_out:1;
+       guint ssl_strict:1;
+       guint trusted_certificate:1;
+       guint clean_dispose:1;
        gpointer ssl_creds;
-       gboolean ssl_strict;
-       gboolean trusted_certificate;
 
        GMainContext   *async_context;
        GSource        *watch_src;
@@ -158,20 +160,32 @@ finalize (GObject *object)
 {
        SoupSocketPrivate *priv = SOUP_SOCKET_GET_PRIVATE (object);
 
-       if (priv->connect_cancel)
+       if (priv->connect_cancel) {
+               if (priv->clean_dispose)
+                       g_warning ("Disposing socket %p during connect", object);
                g_object_unref (priv->connect_cancel);
-       if (priv->iochannel)
+       }
+       if (priv->iochannel) {
+               if (priv->clean_dispose)
+                       g_warning ("Disposing socket %p while still connected", object);
                disconnect_internal (priv);
+       }
 
        if (priv->local_addr)
                g_object_unref (priv->local_addr);
        if (priv->remote_addr)
                g_object_unref (priv->remote_addr);
 
-       if (priv->watch_src)
+       if (priv->watch_src) {
+               if (priv->clean_dispose && !priv->is_server)
+                       g_warning ("Disposing socket %p during async op", object);
                g_source_destroy (priv->watch_src);
-       if (priv->connect_timeout)
+       }
+       if (priv->connect_timeout) {
+               if (priv->clean_dispose)
+                       g_warning ("Disposing socket %p during connect (2)", object);
                g_source_destroy (priv->connect_timeout);
+       }
        if (priv->async_context)
                g_main_context_unref (priv->async_context);
 
@@ -414,6 +428,14 @@ soup_socket_class_init (SoupSocketClass *socket_class)
                                   "Value in seconds to timeout a blocking I/O",
                                   0, G_MAXUINT, 0,
                                   G_PARAM_READWRITE));
+
+       g_object_class_install_property (
+               object_class, PROP_CLEAN_DISPOSE,
+               g_param_spec_boolean ("clean-dispose",
+                                     "Clean dispose",
+                                     "Warn on unclean dispose",
+                                     FALSE,
+                                     G_PARAM_WRITABLE | G_PARAM_CONSTRUCT_ONLY));
 }
 
 
@@ -541,6 +563,9 @@ set_property (GObject *object, guint prop_id,
        case PROP_TIMEOUT:
                priv->timeout = g_value_get_uint (value);
                break;
+       case PROP_CLEAN_DISPOSE:
+               priv->clean_dispose = g_value_get_boolean (value);
+               break;
        default:
                G_OBJECT_WARN_INVALID_PROPERTY_ID (object, prop_id, pspec);
                break;