[Godot] Websocket dropping data violates TCP reliability and congestion control

[joshmossas]

Tested versions

v4.0.beta10.official

System information

N/A

Issue description

See original issue in the Godot repo:
godotengine/godot#70738

The issue has been open since Dec 30, 2022

From the original post:

When the socket is being fed data (receiving) faster than your game can deal with it the WebSocketPeer poll-call will continue to read data despite the buffer filling up and then be forced to drop the data. This is not correct behavior for any application working with any protocol based on TCP.

This is not related to godotengine/godot#22496. If you're receiving websocket messages that are very large, then obviously you'll need to have a buffer large enough to fit such messages. Instead, this is regarding "normal" sized messages but a frame freeze or other performance reducing condition causing the websocket buffer to fill up when it eventually gets to the poll-call.

Dropping data violates the TCP reliability upon which websocket is built which means it's no longer possible to rely on data sent actually being received, causing one to have to implement data re-transmission mechanisms despite those already being present in TCP.

Steps to reproduce

N/A

Minimal reproduction project (MRP)

N/A

[Norrox]

a semi workaround is changing the buffer size manually, but it wont stop the websocketpeer from stalling when its full.

func _ready():
	socket = WebSocketPeer.new()
	socket.connect_to_url("ws://localhost:8765/")
	socket.inbound_buffer_size = NEW SIZE
	socket.outbound_buffer_size = NEW SIZE

[SkogiB]

We have some work being done to fix this, we made a PR upstream to see if Godot will take it first. godotengine/godot#98167

[joshmossas]

@SkogiB Out of curiosity, have you guys determined a criteria for how long is reasonable to wait for a PR to be merged upstream by Godot?

I think it's good that y'all are trying to get some of your fixes merged upstream. As long as it doesn't cause too long of a delay. Having an upper limit of "waiting for Godot" time would probably be a good thing to consider.

Maybe it could even be a variable limit depending on the severity of the issue. Lower severity issues can prolly afford to wait a bit longer, while higher severity issues have a shorter time limit.

IDK just spitballing ideas.

[Norrox]

@SkogiB Out of curiosity, have you guys determined a criteria for how long is reasonable to wait for a PR to be merged upstream by Godot?

I think it's good that y'all are trying to get some of your fixes merged upstream. As long as it doesn't cause too long of a delay. Having an upper limit of "waiting for Godot" time would probably be a good thing to consider.

Maybe it could even be a variable limit depending on the severity of the issue. Lower severity issues can prolly afford to wait a bit longer, while higher severity issues have a shorter time limit.

IDK just spitballing ideas.

#709

[SkogiB]

@SkogiB Out of curiosity, have you guys determined a criteria for how long is reasonable to wait for a PR to be merged upstream by Godot?

I think it's good that y'all are trying to get some of your fixes merged upstream. As long as it doesn't cause too long of a delay. Having an upper limit of "waiting for Godot" time would probably be a good thing to consider.

Maybe it could even be a variable limit depending on the severity of the issue. Lower severity issues can prolly afford to wait a bit longer, while higher severity issues have a shorter time limit.

IDK just spitballing ideas.

We discussed it the other day after receiving some concerns from contributors and changed the practice. We may still submit upstream, but we're not going to expect it of contributors nor bog down our dev cycle waiting on Godot, as that defeats the purpose of the project.

The TCP fix for this is already in the 4.3 betas, as Norrox linked, we were just waiting to see what Godot did with our PR to merge the 4.4 dev one. They introduced their own in-house message handling instead of taking our PR so really we're not sure what to do on 4.4, but for the 4.3 betas it's already in and we got a lot of testing from Chocolate Chip and Keros showing its working well.

[joshmossas]

We discussed it the other day after receiving some concerns from contributors and changed the practice. We may still submit upstream, but we're not going to expect it of contributors nor bog down our dev cycle waiting on Godot, as that defeats the purpose of the project.

Nice I had a similar concern as well so I'm glad y'all have discussed this. Since the fix for this has already been merged into the beta I think it's prolly safe to close this issue. Unless y'all want to keep it open until it gets into a stable release. I guess I'll let y'all decide whether to close or not.

[SkogiB]

Yeah we'll probably close it before long, especially once we decide what do with 4.4dev given that they have their own weird fix that's separate from ours.