godotengine / godot

Godot Engine – Multi-platform 2D and 3D game engine
https://godotengine.org
MIT License
89.8k stars 20.91k forks source link

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

Open Anutrix opened 1 year ago

Anutrix commented 1 year ago

Godot version

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

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 commented 1 year ago

cc @Faless

timothyqiu commented 1 year ago

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:

Anutrix commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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.

ClintonSarkar commented 4 months ago

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().