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

[WS] Fix peer stuck in CLOSING state #101760

Merged
merged 1 commit into from
Jan 20, 2025

Conversation

Faless
Copy link
Collaborator

@Faless Faless commented Jan 18, 2025

This was due by the buffer being cleared on close (including in closing state) preventing further reads.

This commit changes the close logic to only clear the buffer when the peer connection has been fully closed (acknowledged by the other end, or closed due to a "broken" connection).

Fixes #100948

@Faless Faless requested a review from a team as a code owner January 18, 2025 15:00
@Faless Faless added this to the 4.4 milestone Jan 18, 2025
This was due by the buffer being cleared on close (including in closing
state) preventing further reads.

This commit changes the close logic to only clear the buffer when the
peer connection has been fully closed (acknowledged by the other end, or
closed due to a "broken" connection).
@Faless Faless force-pushed the ws/fix_endless_closing branch from af89218 to 81181c5 Compare January 18, 2025 15:02
@Faless Faless changed the title [WS] Fix state stuck in disconnecting [WS] Fix peer stuck in CLOSING state Jan 18, 2025
@mhilbrunner
Copy link
Member

mhilbrunner commented Jan 20, 2025

Wouldn't the same bug lurk here?

void EMWSPeer::close(int p_code, String p_reason) {
if (p_code < 0) {
if (peer_sock != -1) {
godot_js_websocket_destroy(peer_sock);
peer_sock = -1;
}
ready_state = STATE_CLOSED;
}
if (ready_state == STATE_CONNECTING || ready_state == STATE_OPEN) {
ready_state = STATE_CLOSING;
if (peer_sock != -1) {
godot_js_websocket_close(peer_sock, p_code, p_reason.utf8().get_data());
} else {
ready_state = STATE_CLOSED;
}
}
in_buffer.clear();
packet_buffer.clear();
}

(EMWSPeer::close() clearing in_buffer and packet_buffer when STATE_CLOSING instead of only in STATE_CLOSED)

@Faless
Copy link
Collaborator Author

Faless commented Jan 20, 2025

Wouldn't the same bug lurk here?

Not really, at least for now, because we can't control reads on the Web, since the Web API is callback-based (so reads will happen even if the buffer if zero, and we'd be dropping data, but close events will always be processed):

void EMWSPeer::_esws_on_close(void *p_obj, int p_code, const char *p_reason, int p_was_clean) {
EMWSPeer *peer = static_cast<EMWSPeer *>(p_obj);
peer->close_code = p_code;
peer->close_reason.parse_utf8(p_reason);
peer->ready_state = STATE_CLOSED;
}

create: function (socket, p_on_open, p_on_message, p_on_error, p_on_close) {
const id = IDHandler.add(socket);
socket.onopen = GodotWebSocket._onopen.bind(null, id, p_on_open);
socket.onmessage = GodotWebSocket._onmessage.bind(null, id, p_on_message);
socket.onerror = GodotWebSocket._onerror.bind(null, id, p_on_error);
socket.onclose = GodotWebSocket._onclose.bind(null, id, p_on_close);
return id;
},

@Repiteo Repiteo merged commit 7a63dc9 into godotengine:master Jan 20, 2025
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Jan 20, 2025

Thanks!

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

Successfully merging this pull request may close these issues.

[4.4.dev5] Websockets stuck in STATE_CLOSING
4 participants