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

WebSockets don't report the correct state after closing due to "message too big". #97899

Open
DjSapsan opened this issue Oct 6, 2024 · 3 comments · May be fixed by #97913
Open

WebSockets don't report the correct state after closing due to "message too big". #97899

DjSapsan opened this issue Oct 6, 2024 · 3 comments · May be fixed by #97913

Comments

@DjSapsan
Copy link

DjSapsan commented Oct 6, 2024

Tested versions

Godot v4.4.dev2 - supports only TLS1.2 and fails because of lack of 1.3
Godot v4.4.dev (506d6e4) - a newly complied version that is supposed to add support for 1.3 but fails

System information

Ubuntu 24.04.1 LTS 24.04 - X11 - OpenGL 3 (Compatibility) - NVIDIA GeForce RTX 2060 (nvidia; 550.107.02) - AMD Ryzen 9 5900X 12-Core Processor (24 Threads)

Issue description

Can't load information from a server using WSS due to bad TLS1.3 support.
It doesn't work in any version, but this issue is intended for the latest attempts at adding full TLS1.3 support (issue, issue).
So the problem now (as I understand it) is that server sends a lot of data, but Godot can't handle it all. After a short working period it shows errors of ZeroWindow and sometimes others. It means that the client (Godot app) can't receive data fast enough. A browser and Postman still loading the same data perfectly fine.
I think it has something to do with the buffers or maybe something else. How it happens is shown on the screenshot from Wireshark log. Otherwise it's probably working for smaller amount of data, but it's not enough.

image.

It's not a problem on the server side, as the loading is working perfectly fine in the browser and in Postman tests.

Creator of the server allowed to use it

image

Steps to reproduce

Try to use WSS in any version in any way to load data from https://aoe2lobby.com/spectate.
WSS address: wss://aoe2lobby.com/ws/spectate/
Must have headers: var arrayHeaders = PackedStringArray([
"Origin: https://aoe2lobby.com/ws/spectate",
])

Also, an alternative is not working in the same way - wss://aoe2recs.com/dashboard/api/

Minimal reproduction project (MRP)

bad WSS.zip

@Faless
Copy link
Collaborator

Faless commented Oct 7, 2024

The default URL (wss://aoe2lobby.com/ws/spectate) returns a 500 during the handshake, so there is something wrong with the server.

Trying the other url (wss://aoe2recs.com/dashboard/api/) I see it's sending a huge message (172108) which is more than the default max packet / buffer size (65535 bytes).

After changing the inbound_buffer_size to something bigger (e.g. 1 MiB, socket.inbound_buffer_size = (1 << 20) - 1) it seems to work correctly.

In this scenario wslay (the library we use) sends WSLAY_CODE_MESSAGE_TOO_BIG (1009) to the server and close the connection. It appears that we are not correctly updating the internal state to transition to STATE_CLOSING or STATE_CLOSED.

In any case, the problem is that the inbound buffer is smaller than what the server sends. You should increase the inbound buffer of the socket.

@Faless Faless changed the title WebSockets can't handle a higher load (the issue is intended for TLS 1.3 support) WebSockets don't report the correct state after closing due to "message too big". Oct 7, 2024
@Faless Faless linked a pull request Oct 7, 2024 that will close this issue
@DjSapsan
Copy link
Author

DjSapsan commented Oct 7, 2024

nice, after I increased the buffer to 1 MiB it's working now. Huge thanks, I never set it so large before

@DjSapsan
Copy link
Author

DjSapsan commented Oct 7, 2024

@Faless sorry, does it support more than 1 MiB? I Increased it to hundreds of MB bit it's skipping the big load. It's just not disconnecting anymore and then periodically can recieve small updates.

image

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.

3 participants