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

Projects
None yet
2 participants
@mvollrath
Copy link
Contributor

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.

@mvollrath mvollrath requested a review from jihoonl Jan 31, 2019

@jihoonl

This comment has been minimized.

Copy link
Member

commented Feb 1, 2019

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

@mvollrath

This comment has been minimized.

Copy link
Contributor Author

commented Feb 1, 2019

Just tested in a kinetic docker with 4.2.1 👍

@jihoonl

jihoonl approved these changes Feb 1, 2019

@jihoonl jihoonl merged commit 7f2d165 into RobotWebTools:develop Feb 1, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.