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 788723 - Websocket client connection failing with h2o library
Websocket client connection failing with h2o library
Status: RESOLVED FIXED
Product: libsoup
Classification: Core
Component: HTTP Transport
unspecified
Other Linux
: Normal normal
: ---
Assigned To: libsoup-maint@gnome.bugs
libsoup-maint@gnome.bugs
Depends on:
Blocks:
 
 
Reported: 2017-10-09 16:16 UTC by Lionel Landwerlin
Modified: 2018-01-04 12:35 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
session: don't change the Connection type for websocket connections (1.17 KB, patch)
2017-10-09 16:16 UTC, Lionel Landwerlin
none Details | Review
session: don't change the Connection type for websocket connections (1.23 KB, patch)
2017-10-29 13:59 UTC, Lionel Landwerlin
none Details | Review
session: don't request Keep-Alive for upgraded connections (1.28 KB, patch)
2018-01-03 18:00 UTC, Lionel Landwerlin
committed Details | Review

Description Lionel Landwerlin 2017-10-09 16:16:07 UTC
Created attachment 361196 [details] [review]
session: don't change the Connection type for websocket connections

We seem to inject "Connection: Keep-Alive" even for websocket connections.
Comment 1 Claudio Saavedra 2017-10-27 09:04:56 UTC
If I understand correctly, the problem is that keep-alive is not necessary when establishing a WebSocket connections, as then we're no longer using HTTP but the WebSocket protocol instead. If that's the case, this change looks correct to me, but I rather get confirmation from Dan first.
Comment 2 Dan Winship 2017-10-27 14:24:35 UTC
Comment on attachment 361196 [details] [review]
session: don't change the Connection type for websocket connections

>+	    !soup_message_headers_header_contains (item->msg->request_headers,
>+						   "Upgrade", "websocket")) {

actually, make that

	    !soup_message_headers_header_contains (item->msg->request_headers,
						   "Connection", "Upgrade")) {

if we're doing an upgrade, it doesn't matter what protocol we're upgrading to; we don't need to specify "Connection: Keep-Alive" in that case, since that only applies to old HTTP/1.0 servers, and upgrading is an HTTP/1.1 feature.
Comment 3 Lionel Landwerlin 2017-10-29 13:59:08 UTC
Created attachment 362486 [details] [review]
session: don't change the Connection type for websocket connections
Comment 4 Lionel Landwerlin 2017-12-17 22:23:07 UTC
ping?
Comment 5 Claudio Saavedra 2018-01-03 14:59:41 UTC
Review of attachment 362486 [details] [review]:

Sorry for the delay. I think you need to fix the commit message, as your current approach is not to add the keep-alive header in any upgrade, not only for web sockets.
Comment 6 Lionel Landwerlin 2018-01-03 18:00:05 UTC
Created attachment 366257 [details] [review]
session: don't request Keep-Alive for upgraded connections

Thanks!
Comment 7 Claudio Saavedra 2018-01-04 08:40:40 UTC
Review of attachment 366257 [details] [review]:

Thanks!
Comment 8 Lionel Landwerlin 2018-01-04 12:28:15 UTC
Should I push this into the last stable branch too?
Sounds like it could be useful.
Comment 9 Claudio Saavedra 2018-01-04 12:31:47 UTC
Sure.
Comment 10 Lionel Landwerlin 2018-01-04 12:34:59 UTC
Review of attachment 366257 [details] [review]:

Done.
Comment 11 Lionel Landwerlin 2018-01-04 12:35:17 UTC
Thanks!