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

WebSocketPeer.connect_to_url() always returns 0 i.e, OK #81839

Closed
Anutrix opened this issue Sep 18, 2023 · 8 comments · Fixed by #99753
Closed

WebSocketPeer.connect_to_url() always returns 0 i.e, OK #81839

Anutrix opened this issue Sep 18, 2023 · 8 comments · Fixed by #99753

Comments

@Anutrix
Copy link
Contributor

Anutrix commented Sep 18, 2023

Godot version

master - v4.2.dev.custom_build [4df80b0]

System information

Windows 11 Pro 22H2 - Vulkan API 1.3.250 - Forward+ - Using Vulkan Device #0: AMD - Radeon RX 570 Series

Issue description

WebSocketPeer.connect_to_url() always returns 0 i.e, OK regardless of.

Just try:

extends Node

var socket = WebSocketPeer.new()

func _ready():
	print(socket.connect_to_url("abc"))

in any script of any godot project. It will always return 0 i.e, OK.

Steps to reproduce

  1. Run any project having a script that calls the function WebSocketPeer.connect_to_url() and check its return value.
  2. It will print 0 in all cases(bad url, bad connection, non-existent WebSocket server). No errors or warnings are seen in debugger.

Minimal reproduction project

N/A.
Can be reproduced in any project with 2 lines:

var socket = WebSocketPeer.new()
print(socket.connect_to_url("abc"))
@Calinou
Copy link
Member

Calinou commented Sep 18, 2023

cc @Faless

@timothyqiu
Copy link
Member

FYI: abc is a valid URL. Because it's a valid hostname and WebSocketPeer defaults to ws:// when the scheme is missing.

Example invalid URLs:

  • ws:///
  • ws://abc.com:-42/

@Anutrix
Copy link
Contributor Author

Anutrix commented Sep 18, 2023

@timothyqiu Good point. I tried those 2 and they give 31 i.e, ERR_INVALID_PARAMETER. But that's just URL validation. Not sure but string validations usually throw(just implicitly log/print them in case of Godot) errors instead of returning as it is a pre-condition failure, not result of an function/action.
Maybe I am overthinking this.

Also, shouldn't there be some kind of connection failure errors? Getting OK as long as URL is valid might lead to hard to catch bugs in case URL isn't accessible or has a typo. We should add a note in docs at least.
Maybe a timeout after no response for some time makes sense?
Old way of websocketpeer.connection_established.connect(_connected) makes sense now.

@timothyqiu
Copy link
Member

The returned Error is for those who want to make better error report on the UI for user provided URL I think.

Apart from string validation, TLS validation is also done when calling this function since it doesn't take a long time.

Note connection_established is a signal of WebSocketClient, not WebSocketPeer. You can check for various conditions with get_ready_state() after each poll(). But detailed error code/message is still not available, like in 3.x. WebSocketClient is removed in 4.0, but you can use this demo as a reference.

@Faless
Copy link
Collaborator

Faless commented Sep 18, 2023

WebSocketPeer methods are not blocking, as explained by @timothyqiu the URL abc is valid, so the method starts the resolution of it but doesn't wait for it to be resolved, nor it blocks until TCP connect_to_host succeed/fails.
For these reasons, the connect_to_host will most likely then not return OK (unless, the URL is invalid, the peer is already in use, etc).

As mentioned, you should check the value of get_ready_state() to check for state changes.

@Faless Faless added discussion and removed bug labels Sep 18, 2023
@ClintonSarkar
Copy link

I am facing an issue where the get_ready_state() does not update when the network cable connected to the ip the websocket is connected to. I use poll() at the beginning in _process().

@Goury
Copy link

Goury commented Nov 26, 2024

Affects me too.
Please stop not fixing it and fix it already.

@Goury
Copy link

Goury commented Dec 2, 2024

Ok, but the method poll is broken and get_ready_state is getting stuck at 1 even after the socket disconnects

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants