Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unit tests and bug fixes for XmlRpcpp #1214

Closed

Conversation

trainman419
Copy link
Contributor

The initial motivation for this was to fix some rare crashes that we were seeing in xmlrpcpp where it would leak file descriptors (as described in https://answers.ros.org/question/250393/rosout-leaks-file-descriptors/ ) and then would crash if given a socket file descriptor with descriptor number above FD_SETSIZE (commonly 1024)

This also fixes #914, where an incoming XmlRpc request would cause the server to use 100% of a core if it could not successfully call accept(). (replaces #977, #960). There are two fallback mechanisms here:
Measuring the number of open file descriptors and close new incoming connections if the number of free file descriptors falls below a threshold.
If accept fails completely, stop monitoring the server socket for 1 second and then re-add it, so that we can still accept new connections if the old connections eventually close.

  • Fix for file descriptor leaks in XmlRpcClient.
  • Reject incoming connections in XmlRpcServer to prevent file descriptor exhaustion.
  • Timed retry if accept() fails.
  • Fix handling of partial buffer reads and writes in XmlRpcClient.
  • Fix use of uninitialized memory in XmlRpcSocket::get_port.
  • Break dependency on XmlRpc.h header to make targeted mocking easier.
  • Better error reporting when address lookup fails. (Add error message when topic can't connect (roscpp and rospy) #222)
  • Report symbolic state names in XmlRpcClient.
  • Make async XmlRpcClient terminate correctly on error.
  • Update comments to reflect the fact that an XmlRpcDispatch "exception" actually indicates out-of-band TCP data has arrived.
  • Fix XmlRpcDispatch to correctly honor the Exception request again.
  • Make XmlRpcUtil::error go to stdout to match log and provide better error reporting. (xmlrpc should print errors #364)
  • Lots of unit tests.

cc @kmactavish @mikepurvis @mgrrx @ruffsl

* Fix for file descriptor leaks in XmlRpcClient.
* Reject incoming connections in XmlRpcServer to prevent file
descriptor exhaustion.
* Timed retry if accept() fails.
* Fix handling of partial buffer reads and writes in XmlRpcClient.
* Fix use of uninitialized memory in XmlRpcSocket::get_port.
* Break dependency on XmlRpc.h header to make targeted mocking easier.
* Better error reporting when address lookup fails.
* Report symbolic state names in XmlRpcClient.
* Make async XmlRpcClient terminate correctly on error.
* Update comments to reflect the fact that an XmlRpcDispatch "exception" actually indicates out-ob-band TCP data has arrived.
* Fix XmlRpcDispatch to correctly honor the Exception request again.
* Lots of unit tests.
@dirk-thomas
Copy link
Member

The size of the diff is pretty big which makes it rather difficult to review. Can the individual fixes and improvements be split into separate pull requests or at least separate commits?

@trainman419
Copy link
Contributor Author

Ok; I'll try to split this into separate pull requests.

@trainman419
Copy link
Contributor Author

First part of this: #1216 . Many of the other tests depend on that header refactoring, so that will need to merge before I can submit the remainder of my tests and fixes as smaller PRs.

@trainman419 trainman419 closed this Nov 2, 2017
@trainman419
Copy link
Contributor Author

Second part: #1218

@trainman419
Copy link
Contributor Author

Third part: #1221

@trainman419
Copy link
Contributor Author

Fourth part: #1232

@trainman419
Copy link
Contributor Author

Part 5: #1242

@trainman419
Copy link
Contributor Author

Part 6: #1243

@trainman419
Copy link
Contributor Author

Part 7: #1244

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants