ros / ros_comm

ROS communications-related packages, including core client libraries (roscpp, rospy, roslisp) and graph introspection tools (rostopic, rosnode, rosservice, rosparam).
http://wiki.ros.org/ros_comm
761 stars 912 forks source link

Correct reconnection in rospy tcpros_base? #2206

Open MatthijsBurgh opened 2 years ago

MatthijsBurgh commented 2 years ago

Hey,

In a test of rosbridge_library, which is currently failing, I came across a comment about the following line probably should be indented. https://github.com/ros/ros_comm/blob/07fb5469c71e10e4a05fa3a631897d9adece61c5/clients/rospy/src/rospy/impl/tcpros_base.py#L790

And after the quick look in the code, that suggestion could be correct. As right now, it will first try to reconnect. Even if the reconnection is successful, it will still sleep and then return. Only if the reconnection is unsuccesful, it will sleep 2 times longer than the previous sleep and re-enter the while. (In case of unsuccessful reconnection at the first try it will already sleep 1 sec, so it will never sleep 0.5 sec. It only sleeps 0.5 sec after a successful reconnection at the first try.)

So unless there are some undocumented reasons why there should be a sleep after a successful reconnection, I agree that the mentioned line should be indented. (There could be a reason that there should be a sleep before the first try, then the sleep should be at the start of the while. Which will result that it will always start with a sleep of 0.5 sec)

update: I have found that the change in behaviour I noticed is related to https://github.com/ros/ros_comm/commit/b3f8e3c757cb2da35756c95bc16459a372e7874d#diff-2657ef9f5903b672eeb4e9537e18bdc75de0542f6703cc160e184a889a077ff5R810-R814. But that doesn't change my opinion about the suggested change.