GNOME Bugzilla – Bug 598163
session changes for soup-fly improvements
Last modified: 2009-11-23 18:39:50 UTC
currently we have: message is queued -> QUEUED first byte of message gets written -> SENDING last byte of message gets written -> WAITING first byte of response is read -> RECEIVING last byte of response is read -> FINISHED There need to be more states between QUEUED and SENDING. Additional states could be: (functions mentioned are mostly in soup-session-async.c) resolving proxy (from resolve_proxy_addr() to resolved_proxy_addr()) resolving hostname (see below) connecting (from soup_connection_connect_async() to got_connection()) tunneling (from the tunnel part of got_connection() to tunnel_connected()) "resolving proxy" would only happen for people who have proxies, and "tunneling" only for accessing https over proxies, but it would still be good to have timings for them, so we can make sure they aren't slow Those two steps and "connecting" could be implemented just by adding a new signal to SoupSession or SoupSessionAsync, and emitting it at the appropriate points. Resolving hostname is trickier because it happens in SoupSocket inside the guts of the "connecting" step. It's possible to find out when DNS resolution will be needed; SoupSessionAsync can get the SOUP_CONNECTION_REMOTE_ADDRESS off the connection, and call soup_address_is_resolved() on it. But there isn't currently any way for it to know when the resolution is finished. One way to fix thar would be to have update_addrs() in soup-address.c do g_object_notify() on the address property of the addr after setting it, so then the session could watch for that and know when to switch from "resolving" to "connecting". (Should do the same for update_name() too for consistency, although that wouldn't be needed for soup-fly.)
these signals will also be useful for regression tests... (qv bug 596074)
(In reply to comment #0) > Those two steps and "connecting" could be implemented just by adding a new > signal to SoupSession or SoupSessionAsync, and emitting it at the appropriate > points. So do you really prefer the signals to be in SoupSession or should we put them in message to be consistent with all the other signals, which are in SoupMessage? (execept for QUEUED, for obvious reasons).
Created attachment 145939 [details] [review] Add 'connecting' signal to SoupSession Emitted while the session is trying to connect with the server.
OK, so I did one signal as an example, if it looks good I'll move on to the others :) (FWIW, it seems to work fine with soup-fly)
(In reply to comment #2) > So do you really prefer the signals to be in SoupSession or should we put them > in message to be consistent with all the other signals, which are in > SoupMessage? (execept for QUEUED, for obvious reasons). SoupSession, because they are about what the session is doing. If we change the way that the session manages the queue in the future, we shouldn't have to update SoupMessage to have different signals too.
(In reply to comment #4) > OK, so I did one signal as an example, if it looks good I'll move on to the > others :) > > (FWIW, it seems to work fine with soup-fly) Hm... although now that I think about it, it's a bit of a lie. What would be ideal (he said, trying to offload more work onto Xan) would be to have soup-fly have two tables; one with messages, and one with connections. The connections table would have states like "resolving", "connecting", "in use by message X", and "idle". And then in the messages table, you'd go "queued" -> "waiting for a connection" -> "sending" ... Because really, "connecting" isn't a state that applies to messages; when SoupSession decides it needs to create a new connection, it doesn't actually "bind" that connection to the message that it was created for, so you can have something like this happen: queue message #1 to http://example.com/one session creates connection #1, starts connecting finish connecting connection #1 start sending message #1 on connection #1 queue message #2 to http://example.com/two session creates connection #2, starts connecting finish sending message #1, mark connection #1 idle start sending message #2 on connection #1 queue message #3 to http://example.com/three no connection available and can't create a new one finish connecting connection #2 start sending message #3 on connection #2 Using the set of signals I originally described, you'd see msg #1 -> QUEUED -> CONNECTING -> SENDING msg #2 -> QUEUED -> CONNECTING msg #1 -> FINISHED msg #2 -> SENDING msg #3 -> QUEUED (*) msg #2 -> FINISHED msg #3 -> SENDING -> FINISHED So at the (*)ed point, we'd be waiting for connection #2 to finish, but nothing in soup-fly would indicate that. Whereas if we had separate message and connection tables, then we could see that conn #2 was in the CONNECTING state, and msg #3 was in "WAITING_FOR_CONNECTION", and we could see that that makes sense, and so if ephy is hung, it's because the server isn't responding to conn #2. Anyway... that can all be round 2. Just having the new states originally described above would still be an improvement. Might be worth having a "connection-wait" signal/state for the (*) msg #3 case above though? The patch looks fine, but don't bother gtk-doc'ing the signals, since they're basically just for soup-fly, not part of the public API, at least for now.
Created attachment 147285 [details] [review] Add connection states signals to SoupSession This patch adds new signals to SoupSession which allows to handle the state of conections. SoupConnection communicates it's state changes by notifying the change in the state property, and SoupSession has one signal for each possible state. SoupConnection has also a signal which gets emitted when it's message changes.
At https://bugzilla.gnome.org/show_bug.cgi?id=601280 there is avaliable a patch which makes SoupFly to use the new signals.
Comment on attachment 147285 [details] [review] Add connection states signals to SoupSession >@@ -110,6 +113,8 @@ dispose (GObject *object) > SoupConnection *conn = SOUP_CONNECTION (object); > SoupConnectionPrivate *priv = SOUP_CONNECTION_GET_PRIVATE (conn); > >+ g_signal_emit (conn, signals[DESTROYED], 0); You don't need to create a signal for this. You can use g_object_weak_ref() to be notified when an object is disposed. >+ signals[CHANGED_MESSAGE] = I think it would be cleaner to add a new read-only property representing priv->cur_req, and use g_object_notify() when it changes. >+ g_object_class_install_property ( >+ object_class, PROP_STATE, >+ g_param_spec_uint (SOUP_CONNECTION_STATE, That should be g_param_spec_enum. Oh, except you can't because SoupConnectionState isn't a public enum so it's not in soup-enum-types.h. Hm... OK, leave it as uint, but put a comment before it saying: "FIXME: this should be a g_param_spec_enum". >@@ -240,6 +271,9 @@ set_property (GObject *object, guint prop_id, >+ case PROP_STATE: >+ priv->state = g_value_get_uint (value); Should call soup_connection_set_state() rather than doing it directly, for consistent behavior. >+ if (priv->state == SOUP_CONNECTION_IN_USE) { > priv->state = SOUP_CONNECTION_IDLE; >+ } no reason to add {}s there. > priv->state = SOUP_CONNECTION_IDLE; >+ g_object_notify (G_OBJECT(data->conn), "state"); It would probably be better to have a single method that sets priv->state and does the notification, rather than calling g_object_notify() everywhere priv->state changes. (You can probably just use soup_connection_set_state() for this, although you'll need to make sure that the part where it sometimes calls clear_current_request() still does the right thing.) Also, there needs to be a space between "G_OBJECT" and "(". Likewise in lots of other places in this patch, but I'm not going to mention them all. > if (tunnel_addr) { >+ > SoupSessionAsyncTunnelData *data; gratuitous blank line >diff --git a/libsoup/soup-session.c b/libsoup/soup-session.c >+ PROP_STATUS, appears to be unused >+ /** >+ * SoupSession::connecting: Kill this doc comment. (Looks like a leftover from Xan's patch; It's not even the right doc...) >+ signals[CONNECTION_IDLE] = >+ g_signal_new ("connection-idle", You don't need to proxy all of the connection states into session signals; soup-fly will see when new connections are created via the session's "connection-created" signal, and then it can just connect to "notify::state" on each connection itself to see the connection state changes. So get rid of CONNECTION_IDLE, CONNECTION_CONNECTING, CONNECTION_IN_USE, CONNECTION_DISCONNECTED, CONNECTION_CHANGED_MESSAGE, and CONNECTION_DESTROYED. Oh, that would require making SoupConnectionState public though (which would solve the uint/enum property problem). You can put it in soup-misc.h. >+ signals[TUNNELING] = I'd said before that this was needed, but actually, soup-fly could figure it out itself, if it saw a "CONNECT" message get queued... If you do want to keep it, you should fix soup-session-sync.c to emit it at the right time too. (You added it to -async, but not -sync.)
Created attachment 147570 [details] [review] Add connection-created and tunneling signals, SoupConnection notifies state and message I think this new patch solves the problems that had the previous one.
committed to git master with minor changes. thanks.
Thanks for commiting :) Bug 601280 contains patch for improving SoupFly, making it use the new signals and keeping history of previous states of messages and connections.