After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 721343 - Add API to establish proxy-tunnelled connection
Add API to establish proxy-tunnelled connection
Product: libsoup
Classification: Core
Component: API
Other Mac OS
: Normal normal
: ---
Assigned To: libsoup-maint@gnome.bugs
Depends on:
Reported: 2014-01-02 16:00 UTC by Dirkjan Ochtman
Modified: 2015-03-01 15:40 UTC
See Also:
GNOME target: ---
GNOME version: ---

Add flag stay_connected to SoupConnection (2.71 KB, patch)
2014-01-02 16:06 UTC, Dirkjan Ochtman
needs-work Details | Review
Add (private) flag keep_connection to SoupSocket (2.91 KB, patch)
2014-01-02 16:06 UTC, Dirkjan Ochtman
needs-work Details | Review
Add soup_session_proxy_connect() function (7.26 KB, patch)
2014-01-02 16:06 UTC, Dirkjan Ochtman
needs-work Details | Review
soup-connection: no close on dispose (771 bytes, patch)
2014-01-06 13:41 UTC, Dirkjan Ochtman
none Details | Review
soup-socket: add flag close_on_dispose to SoupSocket (2.70 KB, patch)
2014-01-06 13:42 UTC, Dirkjan Ochtman
none Details | Review
soup-session: add soup_session_proxy_connect() function (7.23 KB, patch)
2014-01-06 13:43 UTC, Dirkjan Ochtman
none Details | Review
soup-session: add soup_session_connect{_async,_finish} functions (bug 721343) (8.00 KB, patch)
2014-01-07 14:19 UTC, Dirkjan Ochtman
none Details | Review

Description Dirkjan Ochtman 2014-01-02 16:00:59 UTC
As discussed on IRC.
Comment 1 Dirkjan Ochtman 2014-01-02 16:06:01 UTC
Created attachment 265150 [details] [review]
Add flag stay_connected to SoupConnection
Comment 2 Dirkjan Ochtman 2014-01-02 16:06:25 UTC
Created attachment 265151 [details] [review]
Add (private) flag keep_connection to SoupSocket
Comment 3 Dirkjan Ochtman 2014-01-02 16:06:49 UTC
Created attachment 265152 [details] [review]
Add soup_session_proxy_connect() function
Comment 4 Dan Winship 2014-01-03 21:35:28 UTC
Comment on attachment 265150 [details] [review]
Add flag stay_connected to SoupConnection

>+	g_object_class_install_property (
>+		object_class, PROP_STAY_CONNECTED,
>+		g_param_spec_boolean (SOUP_CONNECTION_STAY_CONNECTED,
>+				  "Stay connected",
>+				  "Whether the connection should stay connected on disposal",

The indentation is a little bit off there. Everything else looks OK.

Actually... "stay-connected" doesn't quite imply that it only means "on dispose". I was expected you to either turn soup_connection_disconnect() into a no-op, or else have it return a warning.

Hm... actually actually... the existing close-on-dispose code is not supposed to ever run; it's an error check, to make sure that SoupSession is properly disconnecting its connections manually before disposing them. And in fact, SoupSession does do that. So... maybe we don't actually need this property at all; you can just remove the close-on-dispose code entirely, and in the normal case, SoupSession will have disconnected the connections itself, and the right thing will happen, and in your case, it *won't* have disconnected them, and the right thing will happen.
Comment 5 Dan Winship 2014-01-03 21:39:58 UTC
Comment on attachment 265151 [details] [review]
Add (private) flag keep_connection to SoupSocket

>+#define SOUP_SOCKET_KEEP_CONNECTION "keep-connection"

I think I'd prefer "close-on-dispose", with the opposite semantics (and it would default to TRUE, and you'd set it FALSE in your case).

> 	guint clean_dispose:1;
> 	guint use_thread_context:1;
> 	gpointer ssl_creds;
>+	gboolean keep_connection;

move the new flag up one line and make it a guint:1 like the other boolean flags

>-	g_clear_object (&priv->iostream);
>+	if (!priv->keep_connection)
>+		g_clear_object (&priv->iostream);

This is wrong; the socket should always be dropping its reference on the iostream; if someone else wants to steal it, they need to add their own ref.

>+		g_value_set_boolean (value, priv->keep_connection);
> 	default:

missing "break;"

>+		g_param_spec_boolean ("keep-connection",
>+				    "Keep connection",

indentation again
Comment 6 Dirkjan Ochtman 2014-01-06 13:41:02 UTC
Created attachment 265434 [details] [review]
soup-connection: no close on dispose

Simplified patch, per comment 4.
Comment 7 Dirkjan Ochtman 2014-01-06 13:42:22 UTC
Created attachment 265435 [details] [review]
soup-socket: add flag close_on_dispose to SoupSocket

Address comments from comment 4.
Comment 8 Dirkjan Ochtman 2014-01-06 13:43:50 UTC
Created attachment 265436 [details] [review]
soup-session: add soup_session_proxy_connect() function

Rebased, fixed-up patch.
Comment 9 Dan Winship 2014-01-06 14:45:31 UTC
Comment on attachment 265152 [details] [review]
Add soup_session_proxy_connect() function

(I'd started this review on Friday and then didn't finish... So this is a review of the original patch, not the updated one.)

>    Add soup_session_proxy_connect() function

Should add at least a little bit of description in the commit message

> 	guint removed              : 1;
> 	guint priority             : 3;
> 	guint ref_count            : 28;
>+	guint force_tunnel         : 1;

move "force_tunnel" to before "priority", and drop the size of ref_count by 1 so that the bits still add up to 32

Er... ugh, no. Actually, "priority" was added in the wrong place; the "private" block is for stuff purely internal to SoupMessageQueue. And "force_tunnel" goes in the public block too. So move both "priority" and "force_tunnel" up to between "async" and "resend_count" (and adjust the sizes of both resend_count and ref_count to compensate).

>-	soup_message_set_https_status (item->msg, item->conn);
>+	g_object_get (G_OBJECT (item->conn), "ssl", &is_ssl, NULL);
>+	if (is_ssl)
>+		soup_message_set_https_status (item->msg, item->conn);

I don't think you need this. soup_message_set_https_status() will still do the right thing if the connection is non-SSL.

>+	g_object_get (G_OBJECT (item->conn), "ssl", &is_ssl, NULL);
>+	if (!is_ssl) {

You should probably add soup_connection_get_ssl().

>-			if (soup_connection_is_tunnelled (item->conn))
>-				tunnel_connect (item);
>-			else
>-				item->state = SOUP_MESSAGE_READY;
>+			if (soup_connection_is_via_proxy (item->conn)) {
>+				if (item->force_tunnel || soup_connection_is_tunnelled (item->conn)) {
>+					tunnel_connect (item);

Hm... so soup_session_proxy_connect() doesn't necessarily use a proxy then? It needs a better name then... (more on this at the end)

>+			if (item->force_tunnel) {
>+				item->state = SOUP_MESSAGE_FINISHING;
>+				break;
>+			}

I think that this check would make more sense in tunnel_complete(); set the state to either READY or FINISHING depending on force_tunnel.

> 			item->state = SOUP_MESSAGE_FINISHED;
>-			soup_message_finished (item->msg);
>+			if (item->force_tunnel)
>+				soup_message_finished (item->msg);

I assume there's a missing "!" there? (You should run "make check" [with all the necessary optional packages installed] after making major changes.)

But anyway, is there any reason to avoid emitting finished in the force_tunnel case?

>-			soup_session_unqueue_item (session, item);
>+			if (!item->force_tunnel)
>+				soup_session_unqueue_item (session, item);

Hm... this is because you want to not have it clear item->conn? It seems like it would be cleaner to just extract item->conn from it before this point.

>+typedef struct {
>+	SoupProxiedCallback callback;
>+	gpointer orig_data;
>+	SoupMessageQueueItem *item;
>+} InterData;

What does "InterData" mean?

But anyway, if you make "soup_session_proxy_connect()" use a GAsyncCallback, then you can use GTask internally to store the callback and user_data, and set the item as the task data, and then you won't need your own struct.

>+void connection_callback(SoupSession *session, SoupMessage *msg, gpointer user_data);
>+void connection_callback(SoupSession *session,
>+					SoupMessage *msg,
>+					gpointer inter_data) {

You don't need to prototype the function. And there should be a space before the "(", and the second and third lines of args are indented incorrectly, and the "{" should be on its own line. (Likewise elsewhere.)

>+void soup_session_proxy_connect(SoupSession *session,

needs docs. (And btw, is @destination expected to be an "http" URI or a "ws" URI?)

So on the name... it looks like the purpose of the function is to either connect directly to @destination, or else to connect via an HTTP proxy if you have an HTTP proxy configured... so maybe soup_session_connect_http()? Or actually, maybe just soup_session_connect()... except if it's going to use GAsyncReadyCallback and gio style async then it should be soup_session_connect_async() and soup_session_connect_finish() (and eventually also soup_session_connect() as a synchronous version).
Comment 10 Dirkjan Ochtman 2014-01-07 14:19:00 UTC
Created attachment 265536 [details] [review]
soup-session: add soup_session_connect{_async,_finish} functions (bug 721343)

New patch. This has most of your comments addressed. One review suggestion you made introduces problems (forcing force_tunnel items from READY to FINISHING from inside the tunnel_complete() function), so I've left that out.
Comment 11 Dirkjan Ochtman 2014-03-05 12:35:06 UTC
Can you please state what needs to be done for this to land? We still want to land this and an accompanying fix in WebKit at some point. It'd be sad if this was left to bitrot.
Comment 12 Dan Winship 2014-03-17 16:26:24 UTC
Hey, so I actually did some hacking on this a little while back. I just pushed my current state to the wip/proxy-connect branch. It's not complete and probably doesn't currently work. (In fact, I'm not even sure it currently compiles.) And unfortunately, I did most of this a month or two ago, so I don't remember exactly why I changed certain things (and it was based on your earlier patches, no the most recent set, so in some places if it looks like I reverted fixes you made, it's actually because I didn't get them to begin with). Oh, and it's built on top of a bunch of other stuff that I'm planning to push to master after branching for gnome-3-12, so you can ignore most of the other stuff between master and the tip of that branch.

So, I definitely am still thinking about this, and planning to get it in for the next release cycle (and also, to rebase the websockets code from bug 627738 to make use of this). If you want to do some more hacking on it before I get to it, the obvious issues at this point are (1) the sync implementation is missing, and (2) there are no test/example programs for it. (And fixing (2) will probably lead to (3) the code got broken in my rewrites :-).
Comment 13 Nicolas Dufresne (ndufresne) 2014-09-24 15:39:52 UTC
Review of attachment 265536 [details] [review]:

::: libsoup/soup-session.c
@@ +4649,3 @@
+ * Since: 2.46
+ */
+void soup_session_connect_async (SoupSession *session,

Ideally, I'd like to see an extra method where you pass in a GIOStream. Otherwise we won't be able to wrap and register this into glib-networking. See g_proxy_connect() signature for explanation. A bit like what is being done for WebSocket would work.
Comment 14 Dan Winship 2014-12-14 14:48:14 UTC
So I just updated the websockets bug (bug 627738), and I'm trying to remember what the exact point of this bug was; websockets ends up not wanting an API that looks exactly like this, because we want to let SoupSession do the WebSocket handshake as an HTTP transaction.

Was the idea behind this bug that it would be used with WebKit's WebSockets code, where you already have code for doing the handshake, so you don't want that part?
Comment 15 Dirkjan Ochtman 2014-12-14 14:56:07 UTC
The idea of this code was just to support proxied WebSockets in libsoup-using WebKit. The easiest way forward (and the way you recommend I do this, IIRC) was implementing this API you came up with.
Comment 16 Dan Winship 2014-12-14 18:45:36 UTC
OK, the websocket branch adds a new API soup_session_steal_connection() that will let you do what you want (it's based on the proxy_connect code); just create a dummy SoupMessage, send it, and when you get the SoupSession::request-started signal for it, call soup_session_steal_connection(). (That signal is emitted just before libsoup starts writing out the request.)
Comment 17 Dan Winship 2015-02-10 11:57:24 UTC
[mass-moving all "UNCONFIRMED" libsoup bugs to "NEW" after disabling the "UNCONFIRMED" status for this product now that allows that. bugspam-libsoup-20150210]
Comment 18 Dan Winship 2015-03-01 15:40:48 UTC
(In reply to Dan Winship from comment #16)
> OK, the websocket branch adds a new API soup_session_steal_connection() that
> will let you do what you want...

and that's now committed