Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WebSocketClient closes connection soon after handshake with some server implementations #27560

Closed
AshfordN opened this issue Mar 31, 2019 · 9 comments · Fixed by #28568
Closed

WebSocketClient closes connection soon after handshake with some server implementations #27560

AshfordN opened this issue Mar 31, 2019 · 9 comments · Fixed by #28568

Comments

@AshfordN
Copy link

@AshfordN AshfordN commented Mar 31, 2019

Godot version:
3.1 stable

OS/device including version:
Ubuntu 18.04 LTS

Issue description:
When connecting to some weboscket servers, the client disconnects immediately after the handshake is complete (i.e. as soon as the server responds to our upgrade request). This, I suspect, is cause by the fact that a Connection: close, Upgrade header is sent in the upgrade request. I also suspect that this is caused by libwebsocket. Below shows the handshake as seen in wireshark.

GET / HTTP/1.1
Pragma: no-cache
Cache-Control: no-cache
Host: echo.websocket.org
Upgrade: websocket
Connection: close, Upgrade
Sec-WebSocket-Key: HKWU1xOVV6PP6HXjcIWMDQ==
Sec-WebSocket-Version: 13

HTTP/1.1 101 Web Socket Protocol Handshake
Connection: Upgrade
Date: Sun, 31 Mar 2019 19:09:01 GMT
Sec-WebSocket-Accept: 0IHc3riAKJz52YmkLVcWrDHvaYs=
Server: Kaazing Gateway
Upgrade: websocket

Steps to reproduce:
Use a WebSocketClient object to connect to one of the following websocket servers:

  • ws://demos.kaazing.com/echo
  • ws://echo.websocket.org

Note that some websocket servers appear to ignore the close field in the connection header. Below is a list of websocket servers that ignore the close field:

Minimal reproduction project:

The following script could be attached to a root node to reproduced the issue, however I have also included a full project setup which can be tested.

extends Node2D

var _client = WebSocketClient.new()

func _ready():
	print("connecting...")
	_client.connect("connection_closed", self, "ws_closed")
	_client.connect("connection_error", self, "ws_connection_error")
	_client.connect("connection_established", self, "ws_connection_established")
	_client.connect("server_close_request", self, "ws_close_request")
	_client.connect_to_url("ws://echo.websocket.org")

func ws_closed(clean):
	if !clean:
		print("websocket closed")
	else:
		print("websocket closed cleanly")

func ws_connection_error():
	print("websocket connection failed")

func ws_connection_established(protocol):
	print("we're connected using protocol: ", protocol)

func ws_close_request(code, reason):
	print("closed with code: ", code, " and reason: ", reason)

func _process(delta):
	if _client.get_connection_status() == WebSocketClient.CONNECTION_DISCONNECTED:
		return
	
	_client.poll()

ws_test.zip

@Faless Faless added this to the 3.2 milestone Mar 31, 2019
@Faless

This comment has been minimized.

Copy link
Contributor

@Faless Faless commented Mar 31, 2019

Maybe related to warmcat/libwebsockets#1435

@AshfordN

This comment has been minimized.

Copy link
Author

@AshfordN AshfordN commented Apr 1, 2019

Yeah, it's definitely related and it seems to be an intended behavior rather than a bug, as per the developer's response here:

warmcat/libwebsockets@082f096#r30944340

The developer states that it's legal, however, I don't see anything in the RFC 6455 that speaks to this particular header form, so it's unclear whether libwebsocket is using a malformed header, or some servers are handling the header incorrectly. I personally think the Connection: close, Upgrade form should be avoided since it's not mentioned in the RFC and it's incompatible with some servers.

@Faless

This comment has been minimized.

Copy link
Contributor

@Faless Faless commented Apr 1, 2019

I personally think the Connection: close, Upgrade form should be avoided since it's not mentioned in the RFC and it's incompatible with some servers.

Combining multiple headers is allowed by HTTP, it should be valid during the WebSocket handshake too:
https://tools.ietf.org/html/rfc2616#section-4.2

It MUST be possible to combine the multiple header fields into one
"field-name: field-value" pair, without changing the semantics of the
message, by appending each subsequent field-value to the first, each
separated by a comma.

That said, a potential solution for Godot 3.1 is to downgrade libwebsockets to v3.0.1 which does not seem affected (reverting #26688, and re-applying the IPv6 fix as done here: https://github.com/Faless/godot/tree/net/lws_rollback).

@Faless Faless changed the title WebSocketClient closes connection soon after handshake WebSocketClient closes connection soon after handshake with some server implementations Apr 1, 2019
@AshfordN

This comment has been minimized.

Copy link
Author

@AshfordN AshfordN commented Apr 1, 2019

Combining multiple headers is allowed by HTTP, it should be valid during the WebSocket handshake too:
https://tools.ietf.org/html/rfc2616#section-4.2

I wasn't questioning the validity of combined headers, I was specifically questioning the use of a Connection: close header when upgrading the connection (regardless of whether or not it's combined).
The Connection: close header is used when the connection is to be closed after a response is received, as per RFC 7230, section 6.1

The "close" connection option is defined for a sender to signal that
this connection will be closed after completion of the response. For
example,

Connection: close

in either the request or the response header fields indicates that
the sender is going to close the connection after the current
request/response is complete

If the websocket protocol, overrode this behavior, it would have stated this explicitly, and it does not, which is why I said the aforementioned header form should be avoided.

@Faless

This comment has been minimized.

Copy link
Contributor

@Faless Faless commented Apr 1, 2019

it would have stated this explicitly, and it does not, which is why I said the aforementioned header form should be avoided.

That may very well be the case, the discussion should be moved upstream

@RyanJEC

This comment has been minimized.

Copy link

@RyanJEC RyanJEC commented Apr 29, 2019

Can we get a fix for this before 3.2 please

@AshfordN

This comment has been minimized.

Copy link
Author

@AshfordN AshfordN commented Apr 30, 2019

This bug is kind of tricky to fix, since the issue has already been raised in the master branch of libwebsocket (the offending library in this case), and the developer insists on keeping the current behavior, even though it doesn't play well with some servers. I personally don't agree with him, since, in my opinion, compatibility is king when it comes to web technologies, regardless of any intuitive justification. Nevertheless, @Faless has suggested that the libwebsocket version used by Godot, be downgraded to v.3.0.1 which is unaffected by the bug. However, I suspect this can't (and won't) be a permanent solution.
Perhaps there's a way Godot can override the Connection header and rewrite it? And if so, users should have the option to enable/disable the close header feature, since is does have some usefulness to it. Unfortunately, I'm too busy and too unfamiliar with the C++ API to explore these options myself, but perhaps another developer could see if the libwebsocket API make this possible, or if it's even a plausible solution to the issue.

@Faless

This comment has been minimized.

Copy link
Contributor

@Faless Faless commented May 1, 2019

Perhaps there's a way Godot can override the Connection header and rewrite it?

I couldn't find one, it seems to me someone has to hack the C library first.

I suggest we downgrade to libwebosckets 3.0.1 for Godot 3.1.
I will try to replace libwebsockets altogether with wslay as soon as I'm done with WebRTC.

@kristjanvalur

This comment has been minimized.

Copy link

@kristjanvalur kristjanvalur commented Jul 13, 2019

I hit this problem myself and there is a workaround: Use the LCCSCF_PIPELINE option on ssl_options when creating a connection and that will disable the "close," part of the Connection: header.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.