GNOME Bugzilla – Bug 744186
Accept new connection from GIOStream
Last modified: 2015-02-20 23:19:05 UTC
For testing reasons, and for interfacing with other kinds of connections (spice-gtk streams in my case), it would be nice to allow connections with a simple GIOStream. It seems SoupSocket is already IOStream friendly, so if it's acceptable idea, I can work on a patch. thanks
[mass-moving all "UNCONFIRMED" libsoup bugs to "NEW" after disabling the "UNCONFIRMED" status for this product now that bugzilla.gnome.org allows that. bugspam-libsoup-20150210]
Can you clarify what you mean? Like, being able to pass a GIOStream to a SoupServer and have it treat it like it came from the server's listening socket?
(In reply to Dan Winship from comment #2) > Can you clarify what you mean? Like, being able to pass a GIOStream to a > SoupServer and have it treat it like it came from the server's listening > socket? that's right, I have a simple proof-of-concept patch for libsoup, but I didn't test it yet. it looks like it could work. It's useful for the spice client, because HTTP connections are multiplexed in the same Spice channel, and we can demultiplex and provide a byte stream to the server (and multiplex from the other side). This would avoid the hacks that exists today in spice-gtk, that require a local port and proxy the connection through a tcp socket (while avoiding external access with some magic).
Created attachment 296650 [details] [review] add soup_server_add_connection() Allow SoupSocket created by client to be accepted as new connection by the SoupServer. This is useful for clients that do not want to rely on system sockets.
Created attachment 296651 [details] [review] Add GIOStream based SoupSocket
Created attachment 296652 [details] [review] tests: add /server/accept/socket This new test shows how to use soup_server_add_connection()
RFC patches, hopefully you like the idea, because it really makes the code much better for spice-gtk embedded webdav server. Thanks!
Comment on attachment 296651 [details] [review] Add GIOStream based SoupSocket This feels like it breaks SoupSocket's invariants too much. Lots of code assumes that (after construction), if either of priv->conn or priv->gsock is set, then the other one is as well, that soup_socket_get_gsocket() does not return NULL, etc. I think having soup_server_add_connection() take a GIOStream but not a SoupSocket makes more sense than having a SoupSocket with no GSocket. You may need to provide some additional arguments to soup_server_add_connection() to give it everything it needs... Be sure to trace all the places the socket gets used from. (Eg, if the SoupServer receives an HTTP/1.0 request with no Host header, then soup-message-server-io.c tries to get an IP address from the SoupSocket [which is one of the things that won't work with this patch].)
Created attachment 297040 [details] [review] add soup_server_add_connection() Allow SoupSocket created by client to be accepted as new connection by the SoupServer. This is useful for clients that do not want to rely on system sockets.
Created attachment 297041 [details] [review] Add GIOStream based SoupSocket
Created attachment 297042 [details] [review] tests: add /server/accept/socket This new test shows how to use soup_server_add_connection()
Created attachment 297043 [details] [review] socket: fix disconnecting with iostream based sockets Rely on priv->conn to decide whether to call disconnect_internal() or not.
Imho, it is best for the API to take a SoupSocket, that the client can configure: I modified the test to show that caller is responsible for setting up a "functionnal" soupsocket to be a new client, and making http/1.0 request. The last patch fixes some disconnection issues that didn't show up before for some reasons but that may be controversial. However,nNo regression should be introduced by those patches. I am still worried about hang reported in bug 744666.
Created attachment 297134 [details] [review] soup-socket: add GIOStream based SoupSocket support
Created attachment 297135 [details] [review] soup-socket: fix disconnecting with iostream based sockets Rely on priv->conn to decide whether to call disconnect_internal() or not.
Created attachment 297136 [details] [review] soup-connection: remove usage of GSocket directly Not strictly required since it's client-side code, but potentially SoupSocket could not have GSocket anymore, and it seems better to use more abstract pollable stream methods.
Created attachment 297137 [details] [review] soup-server: add soup_server_add_connection() Allow SoupSocket created by client to be accepted as new connection by the SoupServer. This is useful for clients that do not want to rely on system sockets for example.
Created attachment 297138 [details] [review] soup-server: mark client context getters as nullable Client should be aware that those functions could return NULL with hand-made SoupSocket clients.
Created attachment 297139 [details] [review] soup-server: add non-GSocket fallbacks for client context If the SoupSocket doesn't have a GSocket, it may still have addresses filled.
Created attachment 297140 [details] [review] tests: add /server/accept/socket This new test shows how to use soup_server_add_connection()
So first, I'm assuming you've (successfully) run "make check" with all optional test-related packages installed... (In reply to Marc-Andre Lureau from comment #14) > Created attachment 297134 [details] [review] [review] > soup-socket: add GIOStream based SoupSocket support The property should be called "iostream", not "giostream". (Yes, GSocket-valued properties/methods/etc are generally called "gsocket", but that's just because "socket" was already used everywhere to mean "SoupSocket", so I wanted to be explicit when I was talking about GSockets. But GIOStreams don't have to worry about that sort of confusion.) (In reply to Marc-Andre Lureau from comment #17) > Created attachment 297137 [details] [review] [review] > soup-server: add soup_server_add_connection() I don't really like this name either, though only for fairly vague reasons (in every other API with _add_ in the name, you're adding something that will remain there until you remove it), and I don't have a great alternative suggestion. soup_server_accept_connection()? Did you have any other naming ideas at any point? Also, I should have noticed this before, but it shouldn't take a SoupSocket, because SoupSocket is basically deprecated (note that all the existing SoupSocket- and SoupAddress-based APIs were deprecated as part of the SoupServer rewrite in 2.48). So the API should be something like: void soup_server_add_connection (SoupServer *server, GIOStream *connection, GSocketAddress *local_addr, GSocketAddress *remote_addr); and then it would create the SoupSocket itself internally (after converting the GSocketAddresses to SoupAddresses, if they were non-NULL). And actually, given this, we don't even need SOUP_SOCKET_IOSTREAM as public API. (SoupSocket hasn't gained any new public API in years.) So you can move the #define to soup-socket-private.h, and drop the gtk-doc documentation. (In reply to Marc-Andre Lureau from comment #18) > Created attachment 297138 [details] [review] [review] > soup-server: mark client context getters as nullable Update the docs to clarify that the return value can only be NULL if you've used soup_server_add_connection()). (In reply to Marc-Andre Lureau from comment #20) > Created attachment 297140 [details] [review] [review] > tests: add /server/accept/socket The indentation is wrong in the GTestIOStream code
(In reply to Dan Winship from comment #21) > So first, I'm assuming you've (successfully) run "make check" with all > optional test-related packages installed... Yes, except two bugs failing randomly before and after the patches: connection-test and context-test. > > (In reply to Marc-Andre Lureau from comment #14) > > Created attachment 297134 [details] [review] [review] [review] > > soup-socket: add GIOStream based SoupSocket support > > The property should be called "iostream", not "giostream". (Yes, > GSocket-valued properties/methods/etc are generally called "gsocket", but > that's just because "socket" was already used everywhere to mean > "SoupSocket", so I wanted to be explicit when I was talking about GSockets. > But GIOStreams don't have to worry about that sort of confusion.) > fixed > > (In reply to Marc-Andre Lureau from comment #17) > > Created attachment 297137 [details] [review] [review] [review] > > soup-server: add soup_server_add_connection() > > I don't really like this name either, though only for fairly vague reasons > (in every other API with _add_ in the name, you're adding something that > will remain there until you remove it), and I don't have a great alternative > suggestion. soup_server_accept_connection()? Did you have any other naming > ideas at any point? Taking a GIOstream, then I think the naming would be best soup_server_accept_iostream(). Changed. > Also, I should have noticed this before, but it shouldn't take a SoupSocket, > because SoupSocket is basically deprecated (note that all the existing > SoupSocket- and SoupAddress-based APIs were deprecated as part of the > SoupServer rewrite in 2.48). So the API should be something like: > > void soup_server_add_connection (SoupServer *server, > GIOStream *connection, > GSocketAddress *local_addr, > GSocketAddress *remote_addr); > > and then it would create the SoupSocket itself internally (after converting > the GSocketAddresses to SoupAddresses, if they were non-NULL). > > And actually, given this, we don't even need SOUP_SOCKET_IOSTREAM as public > API. (SoupSocket hasn't gained any new public API in years.) So you can move > the #define to soup-socket-private.h, and drop the gtk-doc documentation. > done > > (In reply to Marc-Andre Lureau from comment #18) > > Created attachment 297138 [details] [review] [review] [review] > > soup-server: mark client context getters as nullable > > Update the docs to clarify that the return value can only be NULL if you've > used soup_server_add_connection()). done > > (In reply to Marc-Andre Lureau from comment #20) > > Created attachment 297140 [details] [review] [review] [review] > > tests: add /server/accept/socket > > The indentation is wrong in the GTestIOStream code fixed
Created attachment 297399 [details] [review] soup-socket: add GIOStream based SoupSocket support
Created attachment 297400 [details] [review] soup-socket: fix disconnecting with iostream based sockets Rely on priv->conn to decide whether to call disconnect_internal() or not.
Created attachment 297401 [details] [review] soup-connection: remove usage of GSocket directly Not strictly required since it's client-side code, but potentially SoupSocket could not have GSocket anymore, and it seems better to use more abstract pollable stream methods.
Created attachment 297402 [details] [review] soup-server: add soup_server_accept_iostream() Allow a GIOStream to be accepted as new client connection by the SoupServer. This is useful for clients that do not want to rely on system sockets for example.
Created attachment 297403 [details] [review] soup-server: mark client context getters as nullable Client should be aware that those functions could return NULL with hand-made SoupSocket clients.
Created attachment 297404 [details] [review] soup-server: add non-GSocket fallbacks for client context If the SoupSocket doesn't have a GSocket, it may still have addresses filled.
Created attachment 297405 [details] [review] tests: add /server/accept/iostream This new test shows how soup_server_accept_iostream() can be used.
>+ * soup_server_accept_iostream: >+ * @server: a #SoupServer >+ * @stream: a #GIOStream >+ * @local_addr: the local #GSocketAddress associated with the @stream >+ * @remote_addr: the remote #GSocketAddress associated with the @stream last two should have (allow-none) Also, you leak local and remote when they get set. OK to commit once that's fixed.
Attachment 297399 [details] pushed as 892a08b - soup-socket: add GIOStream based SoupSocket support Attachment 297400 [details] pushed as b7bef5d - soup-socket: fix disconnecting with iostream based sockets Attachment 297401 [details] pushed as 7371b82 - soup-connection: remove usage of GSocket directly Attachment 297402 [details] pushed as 50bdc5c - soup-server: add soup_server_accept_iostream() Attachment 297403 [details] pushed as 01b4164 - soup-server: mark client context getters as nullable Attachment 297404 [details] pushed as 1d03dc8 - soup-server: add non-GSocket fallbacks for client context Attachment 297405 [details] pushed as fbf929b - tests: add /server/accept/iostream