rosmaster leaves sockets in CLOSE_WAIT state #610

[mgrrx]

As already discussed in #610, this PR requires extensive testing. This PR replaces the HTTP backend of xmlrpclib by python-requests to fix the HTTP keep-alive problem that has been there for more than one year. This fix does NOT fix all CLOSE_WAIT issues that I observed on our robots (see #831 and #833), it only fixes the problem that is caused by the rosmaster <-> python xmlrpclib communication.

One side effect of the python-requests library ist that it throws completely different exceptions if e.g. the rosmaster is not reachable. I added some catch statements that re-throw the "original" exceptions to not break too many packages that catch the original exceptions.

[dirk-thomas]

Can you please update the imports to something like from rosgraph.xmlrpc import ServerProxy to avoid touching the part where the class is being used. That should make the patch much smaller.

[dirk-thomas]

Please move the line one up to maintain alphabetical order.

[dirk-thomas]

You mentioned that you "added some catch statements that re-throw the "original" exceptions to not break too many packages that catch the original exceptions". Are there any cases where the modified implementation raises different exceptions or behaves differently? Are you expecting to still break some use cases?

[mgrrx]

Yes, the requests library throws totally different exceptions. With requests you'll get an exception derived from requests.RequestException (http://docs.python-requests.org/en/master/_modules/requests/exceptions/#RequestException) where the standard implementation would throw socket.error or maybe also httplib.HTTPExcetion. I added statements to throw a socket.timeout exception for timeouts and socket.error if e.g. the remote port is closed. As you can see in the comment, there are more exceptions possible like InvalidURL or InvalidSchema. If you think that adding this compatibility layer makes sense I can further extend this mapping.

[dirk-thomas]

Since this change should address some serious problems in the current implementation I think it would be great to add. One critical question will be which ROS distro to target though. This PR is made against Indigo which I would almost rule out since it seems to be a too drastic change with too much chance for regressions for a distro already released more then two years ago.

It good target Kinetic but it would still take a lot of testing (hopefully by multiple parties) to ensure that it doesn't break anything.

The most conservative would be to only aim for the to be released L-turtle but that would mean a long time until it is actually being used (and tested in the field).

Therefore I think Kinetic is a reasonable target. We should try to maintain as much of the behavior (with the same exceptions) as reasonably possible. So if you see other extensions not being mapped yet it would be great to add them.

[dirk-thomas]

I would use just raise in this line (without the explicit argument). That carries the intention better imo.

[mikepurvis]

@mgrrx This needs to be moved to target kinetic-devel. I would do it for you, but I don't have write access to your fork. For now going to remove the hitlist label until it merges cleanly (it's messing up some automation I'm working on).

[jspricke]

I will take care.

[jspricke]

rebased to kinetic-devel

[dirk-thomas]

@ros-pull-request-builder retest this please

[mgrrx]

@ros-pull-request-builder retest this please

[dirk-thomas]

I just merged #981, which fixed the failing tests on the kinetic-devel branch. Therefore this should hopefully pass now...

@ros-pull-request-builder retest this please

[dirk-thomas]

The tests fail because the master api now raises a requests.packages.urllib3.exceptions.NewConnectionError if the master is not available. As you already suspected in the original description this needs to be updated to handle the new exception types.

[mgrrx]

I'll have a look tomorrow, thank you!

[mikepurvis]

Okay, so we've now been running this for a few weeks, and we're having issues in our logs with a lot of messages that look like this coming from some rospy processes:

[urllib3.connectionpool][INFO] 2017-02-27 18:15:03,214: Starting new HTTP connection (1): localhost
[urllib3.connectionpool][INFO] 2017-02-27 18:15:03,218: Starting new HTTP connection (1): localhost
[urllib3.connectionpool][INFO] 2017-02-27 18:15:03,223: Starting new HTTP connection (1): localhost
[urllib3.connectionpool][INFO] 2017-02-27 18:15:03,228: Starting new HTTP connection (1): localhost

These seem to be originating from the version of urllib3 that's bundled with Requests, specifically this line:

https://github.com/shazow/urllib3/blob/1.8/urllib3/connectionpool.py#L171

Now, the wrinkle in all this is that we're running all our robots still on Ubuntu Trusty, and we're using the default system version of Requests, which is 2013 vintage. So it's possible this change would be perfectly fine with the Requests that comes with Ubuntu Xenial, but we don't have a great way of testing that at the moment, short of backporting it (which we may well do anyway).

[mikepurvis]

Dropping testing from this for now as we're entering a release sprint— in a few weeks, I'll add it back in in conjunction with testing on an updated python-requests.

[furushchev]

@mikepurvis Any update? I'm very happy if this is merged.

[furushchev]

As I mentioned at #1024, this pull request breaks behavior when rosparam includes non-ascii characters.
Since I could not send pull request to this branch, I had to create another pull request. Sorry for bothering.
Please see explanation at #1024 for detail, and review! :)

[k-okada]

Hi, we have been testing this on Ubuntu Trusty machine and works great for a few days, so it's ok to merge this.
Without this, this we have encountered CLOSE_WAIT trouble every 2 hours (because of our system requires re-start roslaunch often)

[dirk-thomas]

@mgrrx Can you please take a look at the additional patches @furushchev added in #1024 and consider to merge them into your branch in order to add them to this PR.

@k-okada Previous testing has shown some regressions which seemed to be related to this patch. So I don't consider this to be ready to be merge at the moment even if it addresses the CLOSE_WAIT problem for you.

[mikepurvis]

We're now equipped to install an updated requests library onto our bots (via catkin_tools_python), so I'll try to set that up in the next day or two. Sorry for letting this drop.

[furushchev]

Hi, after I tried this patch on our robot and confirmed that with this patch there is no more CLOSE_WAIT after several hundreds spawning / killing nodes.
However I also found 3 points that we need to care about.

• If roscore applied this patch is spawned, some kind of nodes crashes without any errors.
• Also I felt the performance is not so high as the original one at some timing (e.g. rosnode list slightly takes more time than rosmaster without the patch).
• rosnode info /some_node returns all publishers and subscribers, but does not returns their connections

[furushchev]

Hi there,

I further investigated and I found another approach to maintain one rosmaster without crash.
I sent a TCP packet setting ACK flag to all source ports of CLOSE_WAIT connections without payload, and then rosmaster closes the port.
So, if you don't want to change the code base of rosmaster, I'd like to propose to add a function to monitor the number of CLOSE_WAIT connections and ask rosmaster to close them.

And here is python code for this. It looks very dirty and I totally don't have an idea how to integrate this to ros repository though:
https://gist.github.com/8a4b0fa5f7ffe9ff5ea0c69d6c285cc4

[dirk-thomas]

This should be addressed by #1104.

[dirk-thomas]

I will close this for now assuming this patch isn't needed anymore. Please feel to comment and the ticket can be reopened.