Fix/107 #831

mgrrx · 2016-07-13T15:29:32Z

Fixes #107

I replaced the poll() behavior in rospy by epoll() which allows to check for EPOLLRDHUP events. Extract from the man page epoll_ctl:

Stream socket peer closed connection, or shut down writing half of connection. (This flag is especially useful for writing simple code to detect peer shutdown when using Edge Triggered monitoring.)

Using that event we can easily check for closed connections that were hung up by the remote node. I added that check to the regmanager to gracefully close those connections in its update loop. Especially latched publishers suffered from the old behavior as the messages are normally sent only once.

The fix works also on kinetic

dirk-thomas · 2016-08-05T21:37:48Z

clients/rospy/src/rospy/topics.py

@@ -189,7 +193,7 @@ class Poller(object):
    """
    def __init__(self):
        try:
-            self.poller = select.poll()
+            self.poller = select.epoll()


How does this affect cross platform compatibility? The documentation only mentions Linux, what about OS X and Windows?

The Poller class in topics.py provides a unified interface for polling on all three platforms. I cleaned up the code a bit (48f4f0a) and added a fallback to select.poll for Linux < 2.5.44. I did not touch the code for OS X and Windows. So regarding the compatibility I'd say that this PR does not affect the compatibility.

jspricke · 2016-11-12T17:31:05Z

@dirk-thomas anything that keeps this from merging (to kinetic)?

dirk-thomas · 2016-11-14T17:41:23Z

It is mostly due to not having the time to thoroughly test this patch. Hopefully I can get back to this in the near future.

alain-m · 2016-11-17T00:33:00Z

We can reproduce the escalation of open file descriptors - lsof -n | wc -l - and sockets left in CLOSE_WAIT state - sudo netstat -np | grep CLOSE_WAIT | wc -l - on our indigo robots at Savioke during regular navigation tasks. The sockets in question are all associated with python nodes, as expected from reading #107.
We tested this PR on indigo and it effectively fixes the problem for us, thanks @mgrrx . We will test it on kinetic.

alain-m · 2017-01-18T13:41:28Z

Any plans for releasing this fix soon @dirk-thomas? We've been using it on both indigo and kinetic robots since November and haven't found any regression. Let us know if we can help with specific tests.

mgrrx · 2017-01-18T13:48:59Z

@alain-m thanks for testing this.
@mikepurvis I added the hitlist label ;) Can you integrate this PR into your kinetic-edge branch and test it at Clearpath? We've tested this on indigo here at Magazino for quiet some time now without regressions. I can create a PR for kinetic if this helps

dirk-thomas · 2017-01-18T16:41:33Z

Since it has been tested by two parties for a while already I would be fine with merging a PR against kinetic-devel now. The kinetic-edge branch can be rebased against the latest master afterwards to also cover this until the next release.

…nd is related to ros#610 as connections remained in CLOSE_WAIT. Added a check for CLOSE_WAIT connections in the RegManager loop

Added a fallback to select.poll if select.epoll is not available in Linux. OS X will still use kqueue and Windows the noop operator.

Removed whitespace

…rded but the lock is only acquired when connection list needs to be modified. This avoids unnecessary locking.

jspricke · 2017-01-18T19:43:39Z

@dirk-thomas I've moved this to kinetic-devel. No idea why Jenkins has problems with it. Can you have a look and merge it if acceptable?

dirk-thomas · 2017-01-18T19:49:28Z

@ros-pull-request-builder retest this please

dirk-thomas · 2017-01-18T19:59:52Z

test/test_rospy/nodes/listener_once.py

+# ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
+# POSSIBILITY OF SUCH DAMAGE.
+#
+# Revision $Id: gossipbot.py 1013 2008-05-21 01:08:56Z sfkwc $


This line should probably be removed.

dirk-thomas · 2017-01-18T20:00:56Z

I can't say why it didn't trigger the kinetic job after rebasing and "asking Jenkins nicely". I triggered the job manually (http://build.ros.org/view/Kpr/job/Kpr__ros_comm__ubuntu_xenial_amd64/272/) but it won't show up as a build status here when its finished.

dirk-thomas · 2017-01-18T20:03:55Z

Your latest push to the branch triggered a new build anyway. Waiting for the result on that...

dirk-thomas · 2017-01-20T01:23:40Z

test/test_rospy/test/rostest/test_latch_unsubscribe.py

+
+import psutil
+
+from std_msgs.msg import String


I was just about to merge this. But on a last view through the diff I saw that std_msgs is currently neither a runtime nor test dependency yet. Can you please add it as a <test_depend>std_msgs</test_depend>.

... but now it's upset because it was already listed as a build dependency. Maybe just switch this package to format 2?

I am sorry. I am only using format 2 nowadays. Can you please just remove the added test dependency again. No need to update to the new format in this PR.

jspricke · 2017-01-22T06:27:58Z

No problem, done.

clients/rospy/src/rospy/topics.py

mgrrx mentioned this pull request Jul 14, 2016

rosmaster leaves sockets in CLOSE_WAIT state #610 #834

Closed

dirk-thomas reviewed Aug 5, 2016
View reviewed changes

This was referenced Aug 9, 2016

Using poll() in favor of select() in the XmlRPCDispatcher #833

Merged

rospy: get_num_connections() does not drop #813

Closed

mgrrx added the hitlist label Jan 18, 2017

mgrrx added 6 commits January 18, 2017 18:21

Tests for ros#107, currently failing of course

7050923

rospy: switched from poll to epoll for tcpros. Fixes ros#107 for me a…

f637099

…nd is related to ros#610 as connections remained in CLOSE_WAIT. Added a check for CLOSE_WAIT connections in the RegManager loop

using hasattr instead of dir

1f3862e

Simplified the code for selecting the Poller backend.

7380268

Added a fallback to select.poll if select.epoll is not available in Linux. OS X will still use kqueue and Windows the noop operator.

Removed copyrights from tests

72ea719

Removed whitespace

Rewrote the check for closed connections. The crucial part is now gua…

fd679e5

…rded but the lock is only acquired when connection list needs to be modified. This avoids unnecessary locking.

jspricke changed the base branch from indigo-devel to kinetic-devel January 18, 2017 17:23

jspricke changed the base branch from kinetic-devel to indigo-devel January 18, 2017 17:25

jspricke force-pushed the fix/107 branch from 9e58b5f to fd679e5 Compare January 18, 2017 17:30

jspricke changed the base branch from indigo-devel to kinetic-devel January 18, 2017 17:30

dirk-thomas reviewed Jan 18, 2017

View reviewed changes

Remove copyright

29653b6

dirk-thomas reviewed Jan 20, 2017

View reviewed changes

jspricke force-pushed the fix/107 branch from ebee330 to 29653b6 Compare January 22, 2017 06:25

mikepurvis added a commit to clearpathrobotics/ros_comm that referenced this pull request Jan 31, 2017

Fix/107 (ros#831)

fa8659e

mikepurvis added a commit that referenced this pull request Jan 31, 2017

Fix/107 (#831)

acd4976

mikepurvis added a commit that referenced this pull request Jan 31, 2017

Fix/107 (#831)

9b1da8a

mikepurvis added a commit that referenced this pull request Feb 7, 2017

Fix/107 (#831)

975dc85

mikepurvis added a commit that referenced this pull request Feb 7, 2017

Fix/107 (#831)

8065446

mikepurvis added a commit that referenced this pull request Feb 7, 2017

Fix/107 (#831)

2748924

dirk-thomas merged commit c57b173 into ros:kinetic-devel Feb 8, 2017

dirk-thomas mentioned this pull request Mar 2, 2017

changes between 1.11.20 and 1.12.7 for backporting #1008

Merged

dirk-thomas mentioned this pull request Jul 7, 2017

Topic subscription scalability problem #1078

Closed

dirk-thomas reviewed Feb 21, 2020

View reviewed changes

clients/rospy/src/rospy/topics.py Show resolved Hide resolved

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix/107 #831

Fix/107 #831

mgrrx commented Jul 13, 2016 •

edited

Loading

dirk-thomas Aug 5, 2016

mgrrx Aug 8, 2016

jspricke commented Nov 12, 2016

dirk-thomas commented Nov 14, 2016

alain-m commented Nov 17, 2016

alain-m commented Jan 18, 2017

mgrrx commented Jan 18, 2017 •

edited

Loading

dirk-thomas commented Jan 18, 2017

jspricke commented Jan 18, 2017

dirk-thomas commented Jan 18, 2017

dirk-thomas Jan 18, 2017

dirk-thomas commented Jan 18, 2017

dirk-thomas commented Jan 18, 2017 •

edited

Loading

dirk-thomas Jan 20, 2017

jspricke Jan 20, 2017

mikepurvis Jan 22, 2017

dirk-thomas Jan 22, 2017

jspricke commented Jan 22, 2017 via email

Fix/107 #831

Fix/107 #831

Conversation

mgrrx commented Jul 13, 2016 • edited Loading

dirk-thomas Aug 5, 2016

Choose a reason for hiding this comment

mgrrx Aug 8, 2016

Choose a reason for hiding this comment

jspricke commented Nov 12, 2016

dirk-thomas commented Nov 14, 2016

alain-m commented Nov 17, 2016

alain-m commented Jan 18, 2017

mgrrx commented Jan 18, 2017 • edited Loading

dirk-thomas commented Jan 18, 2017

jspricke commented Jan 18, 2017

dirk-thomas commented Jan 18, 2017

dirk-thomas Jan 18, 2017

Choose a reason for hiding this comment

dirk-thomas commented Jan 18, 2017

dirk-thomas commented Jan 18, 2017 • edited Loading

dirk-thomas Jan 20, 2017

Choose a reason for hiding this comment

jspricke Jan 20, 2017

Choose a reason for hiding this comment

mikepurvis Jan 22, 2017

Choose a reason for hiding this comment

dirk-thomas Jan 22, 2017

Choose a reason for hiding this comment

jspricke commented Jan 22, 2017 via email

mgrrx commented Jul 13, 2016 •

edited

Loading

mgrrx commented Jan 18, 2017 •

edited

Loading

dirk-thomas commented Jan 18, 2017 •

edited

Loading