Unit tests and bug fixes for XmlRpcClient

[trainman419]

Add unit tests for and fix the following bugs in XmlRpcClient:

• Fix error handling in XmlRpcClient so that it closes file descriptors and terminates correctly on errors. This fixes the file descriptor leaks reported in https://answers.ros.org/question/250393/rosout-leaks-file-descriptors/
• Fix handling of partial buffer reads and writes in XmlRpcClient.
• Report symbolic state names in XmlRpcClient.
• Make async XmlRpcClient terminate correctly on error.

[dirk-thomas]

%zu should work across all platforms.

[trainman419]

No; z is a GNU-specific size specifier and is not listed in Microsoft's documentation: https://msdn.microsoft.com/en-us/library/tcxf1dw6.aspx

[wjwwood]

I think the z is defined for C++11: http://en.cppreference.com/w/cpp/io/c/fprintf, and it appears to work on Windows (we use it all the time in ROS 2).

I tried this program out on http://webcompiler.cloudapp.net/:

#include <cstdio>

int main()
{
   size_t i = 42;
   printf("%zu\n", i);
   return 0;
}

The output was:

Compiled with  /EHsc /nologo /W4
main.cpp

Compilation successful!

Total compilation time: 328ms

42

Total execution time: 140ms

[trainman419]

We use C++11 internally but it was my understanding that this code still builds in C++03 mode to avoid forcing downstream packages to enable C++11, so I took extra steps to avoid using any C++11 features.

[dirk-thomas]

The .cpp file can be compiled with C++11. We don't want to expose C++11 feature in the headers though since that requires all downstream packages to choose C++11 too.

[dirk-thomas]

After they have been released into Lunar and had no regressions for a while the changes can be considered for backporting into Kinetic. But even for Kinetic a C++11 compiler is a requirement already. See REP 3.

[wjwwood]

That's a fair point, but given that all the modern MSVC compilers use C++11 by default (there is no -std=c++11 like option), and all other compilers support zu without C++11 (that we care about), I think it's safe, in this instance, to just use %zu.

[trainman419]

Updated.

[dirk-thomas]

size_t is unsigned so it needs to be %zu (not %zd).

[trainman419]

Updated.

[dirk-thomas]

Instead of calling close() in the various locations where a read / write fails would it be possible that the calling code actually does that when receiving false instead? Why should this be added here and change the behavior of the API?

[trainman419]

No; there is already a contract between this code and the calling code that determines if the caller should call close on error or not, based on the return value of the setKeepOpen() function:

ros_comm/utilities/xmlrpcpp/src/XmlRpcDispatch.cpp

Lines 157 to 160 in 8b741ac

	if ( ! newMask) {
	_sources.erase(thisIt); // Stop monitoring this one
	if ( ! src->getKeepOpen())
	src->close();

It looks like this is used in the client to keep the socket open after a request is complete so that the socket is available for subsequent requests. I didn't want to change the behavior of that mechanism or interfere with the concept of keeping a client TCP socket open between requests so I opted to keep the logic within the XmlRpcClient class.

I contemplated a few different ways to structure this change:

1. Make the client code call close in the correct scenarios, but this breaks encapsulation and further exposes the client to implementation details of this class.
2. Make the XmlRpcSocket class close the socket on error. This is cleaner, but would require a complete rewrite of XmlRpcSocket to make it own the file descriptor. This was a bigger set of changes than I wanted to make and I thought they would be seen as too invasive to be accepted into ros_comm.
3. Test XmlRpcSocket to make sure that it always returns false when there is an error, and then fix XmlRpcClient so that it always closes the socket when there is an error. I deemed this to be the least invasive and least likely to include changes that could inadvertently change the library behavior.

This is not a breaking change to the API; file descriptors are only closed in places where they were already in an error state, and it's safe to call close multiple times, so client code which calls close again will not fail. Any client which was expecting a persistent connection and which ends up with a broken connection will behave better than the previous implementation. In the previous implementation it would discover the broken socket when attempting to send a new request and would be forced to close the socket and reconnect. In the updated implementation the socket is already closed, so the client immediately knows that it needs to establish a new connection.

[trainman419]

Bump. Is there anything else I can do to keep this moving?

[dirk-thomas]

Is there anything else I can do to keep this moving?

Please see latest pending comment: #1221 (comment)

[trainman419]

Sorry; I missed the comment because it was on an outdated diff. Fixed the format string.

[dirk-thomas]

Thank you for your effort on this.