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 outbound buffer fixes and buffer size query #51036

Merged
merged 1 commit into from
Aug 1, 2021

Conversation

jordo
Copy link
Contributor

@jordo jordo commented Jul 29, 2021

Fixes #50988 on master.

This PR addresses a few issue with outbound buffers in the WebsocketPeer class, and exposes an outbound buffer query for anyone working with a websocket server in godot.

Currently the wslay implementation of WebsocketPeer (anything non HTML5), will buffer an unlimited amount of outgoing messages in wslay_event_queue_msg(). This is effectively an unbound outgoing queue.

When implementing a websocket server, if an outbound websocket's tcp connection is saturated, put_packet will continue to buffer an unlimited amount of outgoing data within the wslay websocket library. This PR will fail put_packet similar to what happens in get_packet if the amount of data being requested to send exceeds the current configured outbound buffer size.

Likewise this PR exposes querying the current amount of buffered outbound data in the websocketpeer buffer. This is useful and needed when implementing a websocket server to implement throttling and congestion control per peer. Normally congestion control would just happen via socket.send() and a stream will self-throttle, however WebsocketPeer is exposed as a PacketPeer interface with messages queuing up in a wslay in-memory queue. With a working outbound buffer and the ability to introspect the amount of current buffered outgoing data, an application can make good decisions about how and when to throttle packets.

The outbound buffer amount is implemented in HTML5 platform using the Websocket.bufferedAmount property https://developer.mozilla.org/en-US/docs/Web/API/WebSocket/bufferedAmount and implemented using wslay_event_get_queued_msg_length in the wslay implementation on other platforms.

Copy link
Collaborator

@Faless Faless left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comments, looks good otherwise 👍 . Great job!

modules/websocket/doc_classes/WebSocketPeer.xml Outdated Show resolved Hide resolved
modules/websocket/wsl_peer.cpp Outdated Show resolved Hide resolved
Copy link
Collaborator

@Faless Faless left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, can you please squash the 2 commits together?
See here for instructions: https://docs.godotengine.org/en/stable/community/contributing/pr_workflow.html#the-interactive-rebase

@jordo
Copy link
Contributor Author

jordo commented Jul 31, 2021

Squashed. Also please see #51037 for 3.X version as well

@akien-mga
Copy link
Member

Would be nice if the commit message could be as clear as the PR title is ("WebsocketPeer outbound buffer fixes and buffer size query") for a nice Git log.

@jordo
Copy link
Contributor Author

jordo commented Aug 1, 2021

modified commit msg.

@akien-mga akien-mga merged commit 8465ecc into godotengine:master Aug 1, 2021
@akien-mga
Copy link
Member

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.

WSLPeer::put_packet has an unbound outgoing buffer. Websocket outgoing buffers unused.
4 participants