Autobahn WebSocket server #426

mvollrath · 2019-08-12T21:46:47Z

Instead of using obscure threading hackery to add flow control to Tornado, use a grown-up WebSocket server with its own flow control features.

mvollrath · 2019-08-12T23:02:26Z

CI pending ros/rosdistro#21999

uahic · 2019-08-14T12:01:32Z

Since Autobahn seems also to have CBOR compression and all of that, do we need my pull request to add the support for compression in service calls then?

mvollrath · 2019-08-14T15:32:48Z

Since Autobahn seems also to have CBOR compression and all of that, do we need my pull request to add the support for compression in service calls then?

This should not affect anything that happens in rosbridge_library, including protocol-level payload compression/encoding.

viktorku

Good work! Left mostly minor comments.

Is it too much work to add some tests that produce significant load such that we see how the autobahn server handles closer-to-real-world scenarios? Like publishing bigger point clouds and/or messages at faster rates. There might be some subtle differences in the shift from Tornado to this Twisted-based WS server so it would be good to make sure this doesn't introduce some regressions.

viktorku · 2019-08-14T17:53:51Z

rosbridge_server/package.xml

@@ -15,9 +15,7 @@
  <maintainer email="jihoonlee.in@gmail.com">Jihoon Lee</maintainer>

  <buildtool_depend>catkin</buildtool_depend>
-  <exec_depend>python-backports.ssl-match-hostname</exec_depend>
-  <exec_depend>python-tornado</exec_depend>
-  <exec_depend>python-twisted-core</exec_depend>


Twisted is still directly used, I think it should stay as a non-transitive dependency.

Good catch, fixed.

viktorku · 2019-08-14T17:55:00Z

rosbridge_server/scripts/rosbridge_websocket.py

+    try:
+        reactor.stop()
+    except ReactorNotRunning:
+        pass


Perhaps some informative log message could go here.

viktorku · 2019-08-14T17:57:11Z

rosbridge_server/scripts/rosbridge_websocket.py

-                application.listen(port, address)
-            rospy.loginfo("Rosbridge WebSocket server started on port %d", port)
+            listenWS(factory, context_factory)
+            rospy.loginfo('listening at {}'.format(uri))


Can we revert the original log format (to be clear it's the rosbridge server listening)?

This was done.

viktorku · 2019-08-14T17:58:52Z

rosbridge_server/src/rosbridge_server/websocket_handler.py

+        self._valve.set()
+
+    def stopProducing(self):
+        rospy.loginfo('stop')


More descriptive logging needed here as well

Oops, this was left here accidentally.

viktorku · 2019-08-14T18:06:16Z

rosbridge_server/src/rosbridge_server/websocket_handler.py

-class RosbridgeWebSocket(WebSocketHandler):
+@implementer(interfaces.IPushProducer)
+class OutgoingValve:
+    """Allows the Autobahn transport to pause outgoing messages from rosbridge."""


Does this detect back-pressure and pauses publishing internally?

Yes, added some more description for this.

is it a good idea to block a publish because of back pressure by default ? If i have a misbehaving client i may optionally want to drop messages and not make all clients suffer. The caller of this code is blocking in nature afaik

It's a good point that there is potential for one slow client to block writing to others. I consider it a flaw in the rosbridge protocol that topics can be subscribed with (and by default) no protocol-level per-client queue, which would effectively decouple clients from the rospy.Subscriber thread. I do think connecting the backpressure by default is good, because it prevents one slow client from running the server out of memory, which in the context of a robotics framework is terrifying.

Until the protocol is fixed, there are two options:

Slow client runs server out of memory.

Slow client blocks other clients subscribed to the same topics.

A third option (really just a mitigation) is to, when subscribing to topics, always set queue_length > 0 to decouple clients from each other. In this case you have to trust the client to not use the default queue_length = 0, but may solve most of your problems as long as you control the application.

rosbridge_server/launch/rosbridge_websocket.launch

Still used directly by the UDP handler.

This was the default in previous versions, but invalid input to Autobahn.

mvollrath · 2019-08-16T15:28:03Z

CI now pending ros/rosdistro#22032 because I forgot to add xenial. Melodic tests are green.

I've done my own anecdotal testing on a couple of different use cases (point clouds and a very busy graph of small messages across many clients) which led to some fixes. I may add some smoke tests for the server as a first step in this PR, mostly to make sure ssl works as configured.

We have access to this information in onOpen.

jihoonl · 2019-08-27T04:55:09Z

It seems like a huge ABI change which makes me concern about backward compatibility. Tornado has been too long to remove immediately.

Instead of replacing, how about making autobahn websocket server as addition to tornado? Then, I would add deprecation sign in tornado server that guides users to use autobahn instead.

This reverts commit 4224671. This create_url method doesn't exist yet in Ubuntu kinetic.

Avoid network collisions and unnecessary configuration while testing.

mvollrath · 2019-08-31T01:30:57Z

It seems like a huge ABI change which makes me concern about backward compatibility. Tornado has been too long to remove immediately.

Instead of replacing, how about making autobahn websocket server as addition to tornado? Then, I would add deprecation sign in tornado server that guides users to use autobahn instead.

I've made the Autobahn server the default and only script entrypoint. The old Tornado handler is still there if anybody was using that interface. The Autobahn server script supports the same features as the Tornado server (and a few more).

dhananjaysathe · 2019-09-12T09:04:30Z

+1 to this PR @jihoonl @mvollrath there are some more issues with the tornado based implementations that needed quite some to track down. Certain clients lik android phones with agressive power saving enabled leasve the websocket in a suspended state hat then chokes up all other clients for messages from the topic thread.
Using this branch and setting pings fixes this as it detects the issue
eg i set : websocket_ping_interval = 5 websocket_ping_timeout=2
i see the log on the server doing as expected:
2019-09-12 08:46:50+0000 [-] dropping connection to peer x.x.x.x with abort=True: WebSocket ping timeout (peer did not respond with pong in time)
this ping pong does not work reliablity in the tornado version either (or maybe the roslibjs missed something)
Great stuff. Do you folks need any help getting this PR in ?

mvollrath · 2019-09-13T13:09:25Z

@dhananjaysathe thanks for your feedback, good to know this is already fixing things for people. 👍

Send 100 large messages in each direction to saturate buffers.

mvollrath · 2019-09-13T14:51:12Z

@viktorku I've made the smoke test run 100 messages of 10,000 byte strings in each direction, which always causes backpressure when I test it here (a protocol queue_length of 1 always drops some messages).

@jihoonl I've preserved the Tornado ABI for compatibility.

jihoonl · 2019-09-15T05:35:55Z

FYI @jspricke @mgrrx, A big change has been merged into develop branch. You may want to check if it breaks your usecase.

jihoonl · 2019-09-16T01:58:40Z

and thanks @mvollrath for nice updates.

mgrrx · 2019-09-16T06:16:55Z

@jihoonl thanks for the heads-up. We'll test it in the coming days.

dirk-thomas · 2019-09-18T00:51:13Z

I would be great of this patch would also be ported to the ros2 branch so that it can benefit from this. I took a look at the diff but am not familiar enough with this to provide the PR myself.

wolfv · 2019-10-22T06:41:02Z

thanks, this is great. the tornado used by rosbridge was getting old and uncompatible with newer tornado versions as used in Jupyter, and websockets were silently failing 👍

moriarty · 2020-04-23T17:08:57Z

It seems like a huge ABI change which makes me concern about backward compatibility. Tornado has been too long to remove immediately.
Instead of replacing, how about making autobahn websocket server as addition to tornado? Then, I would add deprecation sign in tornado server that guides users to use autobahn instead.

I've made the Autobahn server the default and only script entrypoint. The old Tornado handler is still there if anybody was using that interface. The Autobahn server script supports the same features as the Tornado server (and a few more).

It would have been great if this breaking change had waited to go into Noetic, or defaulted the other way around to not introduce the breaking change which was warned about above.

This breaks compatibility with https://github.com/rctoris/jrosbridge
Admittedly it's breaking things which were under specified because javax.websockets isn't setting the scheme in the header Origin.

https://github.com/crossbario/autobahn-python/blob/13949d383af593fb4a78ed21079742c8234d67b1/autobahn/websocket/protocol.py#L96

Adding another proxy between the rosbridge and code that was previously working which adds the protocol to the Origin in the header gets around the issue.

mvollrath requested review from jihoonl and viktorku August 12, 2019 21:46

mvollrath mentioned this pull request Aug 13, 2019

rosbridge_server hanging randomly under load #425

Closed

Autobahn WebSocket server

699cec6

mvollrath force-pushed the autobahn_twisted branch from df72d75 to 699cec6 Compare August 14, 2019 02:44

viktorku reviewed Aug 14, 2019

View reviewed changes

mvollrath commented Aug 15, 2019

View reviewed changes

rosbridge_server/launch/rosbridge_websocket.launch Show resolved Hide resolved

mvollrath added 8 commits August 16, 2019 10:21

Bring back explicit Twisted dependency

c9a1f74

Still used directly by the UDP handler.

Warn when reactor wasn't running at shutdown hook

2d6b526

More descriptive websocket startup log msg

3113d0f

Remove extreneous OutgoingValve stop log

3cb2dd2

More comment for OutgoingValve

8039037

Fix --address parsing as int

fd6e6cd

Use Autobahn's ws url creation util method

4224671

Backwards compatibility for empty websocket addr

1cb627e

This was the default in previous versions, but invalid input to Autobahn.

mvollrath added 2 commits August 16, 2019 18:33

Factor out websocket onConnect

cf3d538

We have access to this information in onOpen.

Add smoke test for rosbridge websocket server

0a03457

mvollrath added 4 commits August 30, 2019 15:40

Revert "Use Autobahn's ws url creation util method"

a84962a

This reverts commit 4224671. This create_url method doesn't exist yet in Ubuntu kinetic.

Preserve RosbridgeWebSocket Tornado ABI

d7bcf4b

Run rosbridge_websocket tests with ephemeral port

9531d25

Avoid network collisions and unnecessary configuration while testing.

Fix broken test create_url on kinetic

3e3c82b

mvollrath requested a review from viktorku August 31, 2019 01:31

Smokier smoke test

2a39946

Send 100 large messages in each direction to saturate buffers.

jihoonl approved these changes Sep 15, 2019

View reviewed changes

jihoonl merged commit 5a0efdc into develop Sep 15, 2019

mvollrath mentioned this pull request Oct 22, 2019

Websocket ping timeout disconnection doesn't work on Kinetic #438

Closed

mikepurvis mentioned this pull request Dec 5, 2019

Autobahn support breaks Python 3. #448

Closed

moriarty mentioned this pull request Apr 23, 2020

Websocket connection not possible through apache2 when updating from 0.11.3 to 0.11.4 #487

Closed

moriarty mentioned this pull request Apr 24, 2020

Autobahn introduced in #426 breaks jrosbridge #488

Closed

pablo-quilez mentioned this pull request Sep 7, 2020

Websocket connection through nginx reverse proxy is not working, probably due to Autobahn introduced in #426 #529

Closed

jtbandes deleted the autobahn_twisted branch August 13, 2021 22:18

zflat mentioned this pull request May 13, 2022

Port #426 Autobahn WebSocket server from ros1 to ros2 #744

Closed

p0rys mentioned this pull request Jun 7, 2022

Stability improvements for multiple clients #741

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Autobahn WebSocket server #426

Autobahn WebSocket server #426

mvollrath commented Aug 12, 2019

mvollrath commented Aug 12, 2019

uahic commented Aug 14, 2019

mvollrath commented Aug 14, 2019

viktorku left a comment

viktorku Aug 14, 2019

mvollrath Aug 16, 2019

viktorku Aug 14, 2019

mvollrath Aug 16, 2019

viktorku Aug 14, 2019

mvollrath Aug 16, 2019

viktorku Aug 14, 2019

mvollrath Aug 16, 2019

viktorku Aug 14, 2019

mvollrath Aug 16, 2019

dhananjaysathe Sep 12, 2019 •

edited

Loading

mvollrath Sep 12, 2019 •

edited

Loading

mvollrath commented Aug 16, 2019

jihoonl commented Aug 27, 2019 •

edited

Loading

mvollrath commented Aug 31, 2019

dhananjaysathe commented Sep 12, 2019

mvollrath commented Sep 13, 2019

mvollrath commented Sep 13, 2019

jihoonl commented Sep 15, 2019

jihoonl commented Sep 16, 2019

mgrrx commented Sep 16, 2019

dirk-thomas commented Sep 18, 2019

wolfv commented Oct 22, 2019

moriarty commented Apr 23, 2020

Autobahn WebSocket server #426

Autobahn WebSocket server #426

Conversation

mvollrath commented Aug 12, 2019

mvollrath commented Aug 12, 2019

uahic commented Aug 14, 2019

mvollrath commented Aug 14, 2019

viktorku left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhananjaysathe Sep 12, 2019 • edited Loading

Choose a reason for hiding this comment

mvollrath Sep 12, 2019 • edited Loading

Choose a reason for hiding this comment

mvollrath commented Aug 16, 2019

jihoonl commented Aug 27, 2019 • edited Loading

mvollrath commented Aug 31, 2019

dhananjaysathe commented Sep 12, 2019

mvollrath commented Sep 13, 2019

mvollrath commented Sep 13, 2019

jihoonl commented Sep 15, 2019

jihoonl commented Sep 16, 2019

mgrrx commented Sep 16, 2019

dirk-thomas commented Sep 18, 2019

wolfv commented Oct 22, 2019

moriarty commented Apr 23, 2020

dhananjaysathe Sep 12, 2019 •

edited

Loading

mvollrath Sep 12, 2019 •

edited

Loading

jihoonl commented Aug 27, 2019 •

edited

Loading