[xmlrpcpp] close socket when connection can't be accepted. Fixes #914

[mgrrx]

This PR fixes #914. As suggested by @trainman419 and confirmed by @ruffsl, uncommenting line

ros_comm/utilities/xmlrpcpp/src/XmlRpcServer.cpp

Line 157 in ea13207

//this->close();

did the trick.
I also changed the return value of handleEvent in that case.

[jspricke]

Looks like this has been there forever (at least I didn't find an argument against it):
http://xmlrpcpp.cvs.sourceforge.net/viewvc/xmlrpcpp/xmlrpc%2B%2B/src/XmlRpcServer.cpp?revision=1.1&view=markup
So +1 for fixing :).

[mikepurvis]

I wish we had a test or something to exercise this execution path. It seems really hinky to have a "fix" that's essentially just uncommenting something previously commented.

[mgrrx]

I carefully checked when this statement is executed. Basically, it's only called if ::accept returns -1. In my opinion, all the errors described in the manpage suggest to close the file descriptor

[mikepurvis]

If you've confirmed in #914 that this eliminates a source of leaked sockets, then that's enough motivation for me. +1 to merge for now; we can keep an eye out for any regressions due to this in our testing at CPR.

[dirk-thomas]

Sounds good to me.

[mikepurvis]

We're seeing some assertion failure crashes now which may be due to this change. This one is a stack trace for one which occurred in the context of a costmap_2d unit test run:

  File "../nptl/sysdeps/unix/sysv/linux/raise.c", line 56, in __GI_raise
  File "abort.c", line 89, in __GI_abort
  File "../sysdeps/posix/libc_fatal.c", line 175, in __libc_message
  File "fortify_fail.c", line 38, in __GI___fortify_fail
  File "chk_fail.c", line 28, in __GI___chk_fail
  File "fdelt_chk.c", line 25, in __fdelt_chk
  File "../../../../../ros-comm-f6da1ec87d6ed4653e65ac2dbae1325f7bc1238d/utilities/xmlrpcpp/src/XmlRpcDispatch.cpp", line 100, in XmlRpc::XmlRpcDispatch::work
  File "../../../../../ros-comm-f6da1ec87d6ed4653e65ac2dbae1325f7bc1238d/utilities/xmlrpcpp/src/XmlRpcServer.cpp", line 133, in XmlRpc::XmlRpcServer::work
  File "../../../../../ros-comm-f6da1ec87d6ed4653e65ac2dbae1325f7bc1238d/clients/roscpp/src/libros/xmlrpc_manager.cpp", line 272, in ros::XMLRPCManager::serverThreadFunc
  File "/usr/lib/x86_64-linux-gnu/libboost_thread.so.1.54.0", in ?? ()
  File "pthread_create.c", line 312, in start_thread
  File "../sysdeps/unix/sysv/linux/x86_64/clone.S", line 111, in clone

@jasonimercer @deng02

[mikepurvis]

@dirk-thomas @mgrrx We haven't been able to bottom this out, but it's impacting our ability to test everything else. IMO it should be reverted until it can be proven safe.

[dirk-thomas]

I completely agree with your conclusion.