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 792212 - Add API to create a new connection from a SoupSession
Add API to create a new connection from a SoupSession
Status: RESOLVED FIXED
Product: libsoup
Classification: Core
Component: API
unspecified
Other Linux
: Normal normal
: ---
Assigned To: libsoup-maint@gnome.bugs
libsoup-maint@gnome.bugs
Depends on:
Blocks:
 
 
Reported: 2018-01-04 17:17 UTC by Carlos Garcia Campos
Modified: 2018-02-01 14:21 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Add new API to create a new connection from a SoupSession (19.14 KB, patch)
2018-01-04 17:19 UTC, Carlos Garcia Campos
none Details | Review
Add new API to create a new connection from a SoupSession (20.68 KB, patch)
2018-01-05 07:21 UTC, Carlos Garcia Campos
none Details | Review
Add new API to create a new connection from a SoupSession (20.68 KB, patch)
2018-01-08 09:53 UTC, Carlos Garcia Campos
none Details | Review
Add new API to create a new connection from a SoupSession (20.91 KB, patch)
2018-01-24 09:14 UTC, Carlos Garcia Campos
committed Details | Review

Description Carlos Garcia Campos 2018-01-04 17:17:22 UTC
We need this API in WebKit for the WebSockets implementation. We currently use GSocketClient directly to create the connection, which doesn't take into account the current session properties like proxy settings. We can't use libsoup Websockets API, because WebKit implements it, we only need a lower level API to establish the connection, without sending anything to the server.
Comment 1 Carlos Garcia Campos 2018-01-04 17:19:34 UTC
Created attachment 366321 [details] [review]
Add new API to create a new connection from a SoupSession
Comment 2 Carlos Garcia Campos 2018-01-05 07:21:40 UTC
Created attachment 366358 [details] [review]
Add new API to create a new connection from a SoupSession

I've factored out the code to steal the connection to remove duplicated code.
Comment 3 Michael Catanzaro 2018-01-05 20:35:00 UTC
Review of attachment 366358 [details] [review]:

I don't pretend to understand SoupSession... just a couple comments about the docs:

::: libsoup/soup-session.c
@@ +4906,3 @@
+ *
+ * Prototype for the progress callback passed to soup_session_connect_async(),
+ * qv.

qv?

@@ +4975,3 @@
+ * @cancellable: a #GCancellable
+ * @progress_callback: (allow-none) (scope async): a #SoupSessionConnectProgressCallback which
+ * will be called for every network event occurred during the connection.

every network event that occurs during the connection.
Comment 4 Carlos Garcia Campos 2018-01-08 09:41:59 UTC
(In reply to Michael Catanzaro from comment #3)
> Review of attachment 366358 [details] [review] [review]:
> 
> I don't pretend to understand SoupSession... just a couple comments about
> the docs:
> 
> ::: libsoup/soup-session.c
> @@ +4906,3 @@
> + *
> + * Prototype for the progress callback passed to
> soup_session_connect_async(),
> + * qv.
> 
> qv?

No idea, I copied it from other documentation blocks.

> @@ +4975,3 @@
> + * @cancellable: a #GCancellable
> + * @progress_callback: (allow-none) (scope async): a
> #SoupSessionConnectProgressCallback which
> + * will be called for every network event occurred during the connection.
> 
> every network event that occurs during the connection.

Ok, thanks!
Comment 5 Carlos Garcia Campos 2018-01-08 09:53:49 UTC
Created attachment 366481 [details] [review]
Add new API to create a new connection from a SoupSession

Addressed review comments.
Comment 6 Dan Winship 2018-01-08 22:29:59 UTC
(In reply to Michael Catanzaro from comment #3)
> + * Prototype for the progress callback passed to
> soup_session_connect_async(),
> + * qv.
> 
> qv?

You kids these days with your not knowing classical Latin abbreviations!

"qv" = "quod vide" = "[regarding] which, see"

ie, "Prototype for the progress callback passed to soup_session_connect_async(), which you should consult for further details".
Comment 7 Dan Winship 2018-01-08 22:42:45 UTC
I'm not convinced this patch is needed. You said in the WebKit bug (https://bugs.webkit.org/show_bug.cgi?id=126384):

> We can't use
> soup_session_steal_connection() either, because you are expected to steal
> the connection when upgrading the protocol once 101 response is returned
> from the server.

But I don't think that's true. Maybe that's how soup_session_websocket_connect_async() does it, but if so, that's just because that's what was convenient for the websockets implementation. You ought to be able to steal the connection at any point in the process...
Comment 8 Carlos Garcia Campos 2018-01-09 08:40:05 UTC
(In reply to Dan Winship from comment #7)
> I'm not convinced this patch is needed. You said in the WebKit bug
> (https://bugs.webkit.org/show_bug.cgi?id=126384):
> 
> > We can't use
> > soup_session_steal_connection() either, because you are expected to steal
> > the connection when upgrading the protocol once 101 response is returned
> > from the server.
> 
> But I don't think that's true. Maybe that's how
> soup_session_websocket_connect_async() does it, but if so, that's just
> because that's what was convenient for the websockets implementation. You
> ought to be able to steal the connection at any point in the process...

I tried hard to do so, and I couldn't because there isn't a way to steal the connection at the right moment, before the message io is created in this case. So, we would need new API in soup in any case, to add another signal to the session or message that allows to call steal at that moment. Then I thought that if we had to add new API it would be better to add an explicit call to create a connection. It's a lot easier to understand than creating a message, connect to a signal, start a request, steal the connection in the signal and cancel the request.
Comment 9 Claudio Saavedra 2018-01-10 09:13:46 UTC
Review of attachment 366481 [details] [review]:

A few minor comments from reading the patch.

::: libsoup/soup-session.c
@@ +4924,3 @@
+
+        data = g_slice_new (ConnectAsyncData);
+typedef struct {

That there is no ref here when the free() function is unreffing is a bit unclean, but since this is internal and there's only one use of it I guess it's OK?

@@ +4978,3 @@
+ * @user_data: data for @progress_callback and @callback
+ *
+}

The comment needs to be wrapped.

@@ +5010,3 @@
+        g_signal_connect_object (msg, "network-event",
+                                 G_CALLBACK (connect_async_message_network_event),
+                                GTask       *task)

You probably can skip connecting to this signal if there is no progress callback.
Comment 10 Carlos Garcia Campos 2018-01-11 14:37:44 UTC
(In reply to Claudio Saavedra from comment #9)
> Review of attachment 366481 [details] [review] [review]:
> 
> A few minor comments from reading the patch.
> 
> ::: libsoup/soup-session.c
> @@ +4924,3 @@
> +
> +        data = g_slice_new (ConnectAsyncData);
> +typedef struct {
> 
> That there is no ref here when the free() function is unreffing is a bit
> unclean, but since this is internal and there's only one use of it I guess
> it's OK?

It's transferring the ownership, but yes, we can simply ref in the constructor and unref in the caller.

> @@ +4978,3 @@
> + * @user_data: data for @progress_callback and @callback
> + *
> +}
> 
> The comment needs to be wrapped.

Why?

> @@ +5010,3 @@
> +        g_signal_connect_object (msg, "network-event",
> +                                 G_CALLBACK
> (connect_async_message_network_event),
> +                                GTask       *task)
> 
> You probably can skip connecting to this signal if there is no progress
> callback.

Good point, I'll do it.
Comment 11 Claudio Saavedra 2018-01-11 15:20:31 UTC
(In reply to Carlos Garcia Campos from comment #10)

> > @@ +4978,3 @@
> > + * @user_data: data for @progress_callback and @callback
> > + *
> > +}
> > 
> > The comment needs to be wrapped.
> 
> Why?

Easier to read, in bugzilla's review tool for example :)
Comment 12 Carlos Garcia Campos 2018-01-24 09:14:25 UTC
Created attachment 367358 [details] [review]
Add new API to create a new connection from a SoupSession

Updated patch. Dan, are you opposed to this approach? The other solution would be to make it possible use the existing steal method at any time and adding a new signal to be emitted right after the connection is done and before the request is sent to call steal. I still think this new API is a lot simpler to use, FWIW.
Comment 13 Dan Winship 2018-01-26 15:17:41 UTC
(In reply to Carlos Garcia Campos from comment #12)
> Updated patch. Dan, are you opposed to this approach?

I'm not the one who's going to be maintaining it long term.

> The other solution
> would be to make it possible use the existing steal method at any time and
> adding a new signal to be emitted right after the connection is done and
> before the request is sent to call steal.

SoupSession:request-starting / SoupMessage:starting are exactly that, I think.

> I still think this new API is a lot simpler to use, FWIW.

Certainly true, because it applies only to this one situation. steal_connection has to be more complicated because it was designed to let you take over at any point.

Anyway, I still think the WebKit bug can be fixed without any new API, but if this makes things vastly simpler then go ahead.
Comment 14 Carlos Garcia Campos 2018-01-26 15:41:29 UTC
(In reply to Dan Winship from comment #13)
> (In reply to Carlos Garcia Campos from comment #12)
> > Updated patch. Dan, are you opposed to this approach?
> 
> I'm not the one who's going to be maintaining it long term.

:-)

> > The other solution
> > would be to make it possible use the existing steal method at any time and
> > adding a new signal to be emitted right after the connection is done and
> > before the request is sent to call steal.
> 
> SoupSession:request-starting / SoupMessage:starting are exactly that, I
> think.

I guess you mean request-started. No, they aren't. It's too late in both cases, because the request is sent to the server right after the signals unconditionally. In the case of WebKit, we need the connection before the request is made, when there isn't any message io yet and making sure nothing is sent, because the request is created and sent by WebKit, we only need a low-level connection (socket). To use those signals we would need a way to effectively cancel the message at that point.

> > I still think this new API is a lot simpler to use, FWIW.
> 
> Certainly true, because it applies only to this one situation.
> steal_connection has to be more complicated because it was designed to let
> you take over at any point.

Not exactly, steal_connection assumes that the message io already exists, so it's designed to steal the connection after the request has been sent to the server.

> Anyway, I still think the WebKit bug can be fixed without any new API, but
> if this makes things vastly simpler then go ahead.

I think I tried everything, but I couldn't make it work without adding new API to libsoup.
Comment 15 Claudio Saavedra 2018-02-01 13:12:31 UTC
Review of attachment 367358 [details] [review]:

For all I can see the patch looks fine, and since I trust Carlos' assessment that this seems to be the only way to solve the WK bug, I say let's do this.