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

Synchronous websocket write #385

Merged
merged 1 commit into from
Feb 1, 2019

Conversation

mvollrath
Copy link
Contributor

@mvollrath mvollrath commented Jan 31, 2019

Fixes #212 by blocking send_message() while a message is being written.

In practice this connects the backpressure to either the rospy.Subscriber Thread (in the case of MessageHandler or ThrottleMessageHandler) or the QueueMessageHandler Thread in that case.

Applications should subscribe with queue_length of at least 1 to prevent pushing all the way back to the rospy.Subscriber, which is probably rude to other clients, and/or use the "workersocket" transportLibrary to decouple the WebSocket from the client main thread.

There's still a way to DoS other clients (0 queue_length, never ACK) but this is a net positive for the websocket server:

  • QueueMessageHandler actually works instead of dumping messages into Tornado's limitless queue immediately.
  • Server doesn't OOM when the client can't keep up with incoming data.

@jihoonl
Copy link
Member

jihoonl commented Feb 1, 2019

It looks good. Will this work with tornado 4.2.1(the default tornado version in xenial)?

@mvollrath
Copy link
Contributor Author

Just tested in a kinetic docker with 4.2.1 👍

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

Successfully merging this pull request may close these issues.

None yet

2 participants