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.

Sign up for GitHub

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 XmlRpcClient #1221

Merged
merged 3 commits into from
Nov 10, 2017
Merged

Unit tests and bug fixes for XmlRpcClient #1221

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4efa61b
Unit tests and bug fixes for XmlRpcClient
trainman419 Nov 8, 2017
7bc58ec
Use %zd for size_t format string
trainman419 Nov 9, 2017
cfd046c
Update format string for unsinged size_t
trainman419 Nov 10, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 8 additions & 4 deletions utilities/xmlrpcpp/include/xmlrpcpp/XmlRpcClient.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ namespace XmlRpc {
virtual bool setupConnection();

virtual bool generateRequest(const char* method, XmlRpcValue const& params);
virtual std::string generateHeader(std::string const& body);
virtual std::string generateHeader(size_t length) const;
virtual bool writeRequest();
virtual bool readHeader();
virtual bool readResponse();
Expand All @@ -90,6 +90,8 @@ namespace XmlRpc {
enum ClientConnectionState { NO_CONNECTION, CONNECTING, WRITE_REQUEST, READ_HEADER, READ_RESPONSE, IDLE };
ClientConnectionState _connectionState;

static const char * connectionStateStr(ClientConnectionState state);

// Server location
std::string _host;
std::string _uri;
Expand All @@ -107,6 +109,8 @@ namespace XmlRpc {
// Number of times the client has attempted to send the request
int _sendAttempts;

// NOTE(austin): Having multiple variables that imply that the fd is valid
// smells funny.
// Number of bytes of the request that have been written to the socket so far
int _bytesWritten;

Expand All @@ -116,9 +120,9 @@ namespace XmlRpc {

// True if the server closed the connection
bool _eof;
// True if a fault response was returned by the server
bool _isFault;

// True if a fault response was returned by the server
bool _isFault;

// Number of bytes expected in the response body (parsed from response header)
int _contentLength;
Expand Down
84 changes: 64 additions & 20 deletions utilities/xmlrpcpp/src/XmlRpcClient.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,8 @@
#include "xmlrpcpp/XmlRpcClient.h"

#include "xmlrpcpp/XmlRpcSocket.h"
#include "xmlrpcpp/XmlRpc.h"
#include "xmlrpcpp/XmlRpcUtil.h"
#include "xmlrpcpp/XmlRpcValue.h"

#include <stdio.h>
#include <stdlib.h>
Expand All @@ -28,6 +29,24 @@ const char XmlRpcClient::METHODRESPONSE_TAG[] = "<methodResponse>";
const char XmlRpcClient::FAULT_TAG[] = "<fault>";


const char * XmlRpcClient::connectionStateStr(ClientConnectionState state) {
switch(state) {
case NO_CONNECTION:
return "NO_CONNECTION";
case CONNECTING:
return "CONNECTING";
case WRITE_REQUEST:
return "WRITE_REQUEST";
case READ_HEADER:
return "READ_HEADER";
case READ_RESPONSE:
return "READ_RESPONSE";
case IDLE:
return "IDLE";
default:
return "UNKNOWN";
}
}

XmlRpcClient::XmlRpcClient(const char* host, int port, const char* uri/*=0*/)
{
Expand Down Expand Up @@ -79,7 +98,7 @@ struct ClearFlagOnExit {
bool
XmlRpcClient::execute(const char* method, XmlRpcValue const& params, XmlRpcValue& result)
{
XmlRpcUtil::log(1, "XmlRpcClient::execute: method %s (_connectionState %d).", method, _connectionState);
XmlRpcUtil::log(1, "XmlRpcClient::execute: method %s (_connectionState %s).", method, connectionStateStr(_connectionState));

// This is not a thread-safe operation, if you want to do multithreading, use separate
// clients for each thread. If you want to protect yourself from multiple threads
Expand Down Expand Up @@ -118,7 +137,7 @@ XmlRpcClient::execute(const char* method, XmlRpcValue const& params, XmlRpcValue
bool
XmlRpcClient::executeNonBlock(const char* method, XmlRpcValue const& params)
{
XmlRpcUtil::log(1, "XmlRpcClient::execute: method %s (_connectionState %d).", method, _connectionState);
XmlRpcUtil::log(1, "XmlRpcClient::executeNonBlock: method %s (_connectionState %s).", method, connectionStateStr(_connectionState));

// This is not a thread-safe operation, if you want to do multithreading, use separate
// clients for each thread. If you want to protect yourself from multiple threads
Expand Down Expand Up @@ -146,8 +165,16 @@ XmlRpcClient::executeCheckDone(XmlRpcValue& result)
{
result.clear();
// Are we done yet?
if (_connectionState != IDLE)
// If we lost connection, the call failed.
if (_connectionState == NO_CONNECTION) {
return true;
}

// Otherwise, assume the call is still in progress.
if (_connectionState != IDLE) {
return false;
}

if (! parseResponse(result))
{
// Hopefully the caller can determine that parsing failed.
Expand All @@ -168,8 +195,9 @@ XmlRpcClient::handleEvent(unsigned eventType)
XmlRpcUtil::error("Error in XmlRpcClient::handleEvent: could not connect to server (%s).",
XmlRpcSocket::getErrorMsg().c_str());
else
XmlRpcUtil::error("Error in XmlRpcClient::handleEvent (state %d): %s.",
_connectionState, XmlRpcSocket::getErrorMsg().c_str());
XmlRpcUtil::error("Error in XmlRpcClient::handleEvent (state %s): %s.",
connectionStateStr(_connectionState),
XmlRpcSocket::getErrorMsg().c_str());
return 0;
}

Expand Down Expand Up @@ -275,7 +303,7 @@ XmlRpcClient::generateRequest(const char* methodName, XmlRpcValue const& params)
}
body += REQUEST_END;

std::string header = generateHeader(body);
std::string header = generateHeader(body.length());
XmlRpcUtil::log(4, "XmlRpcClient::generateRequest: header is %d bytes, content-length is %d.",
header.length(), body.length());

Expand All @@ -285,7 +313,7 @@ XmlRpcClient::generateRequest(const char* methodName, XmlRpcValue const& params)

// Prepend http headers
std::string
XmlRpcClient::generateHeader(std::string const& body)
XmlRpcClient::generateHeader(size_t length) const
{
std::string header =
"POST " + _uri + " HTTP/1.1\r\n"
Expand All @@ -295,20 +323,12 @@ XmlRpcClient::generateHeader(std::string const& body)
header += _host;

char buff[40];
#ifdef _MSC_VER
sprintf_s(buff,40,":%d\r\n", _port);
#else
sprintf(buff,":%d\r\n", _port);
#endif
snprintf(buff,40,":%d\r\n", _port);

header += buff;
header += "Content-Type: text/xml\r\nContent-length: ";

#ifdef _MSC_VER
sprintf_s(buff,40,"%d\r\n\r\n", (int)body.size());
#else
sprintf(buff,"%d\r\n\r\n", (int)body.size());
#endif
snprintf(buff,40,"%zu\r\n\r\n", length);

return header + buff;
}
Expand All @@ -322,6 +342,8 @@ XmlRpcClient::writeRequest()
// Try to write the request
if ( ! XmlRpcSocket::nbWrite(this->getfd(), _request, &_bytesWritten)) {
XmlRpcUtil::error("Error in XmlRpcClient::writeRequest: write error (%s).",XmlRpcSocket::getErrorMsg().c_str());
// If the write fails, we had an unrecoverable error. Close the socket.
close();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return false;
}

Expand All @@ -332,6 +354,11 @@ XmlRpcClient::writeRequest()
_header = "";
_response = "";
_connectionState = READ_HEADER;
} else {
// On partial write, remove the portion of the output that was written from
// the request buffer.
_request = _request.substr(_bytesWritten);
_bytesWritten = 0;
}
return true;
}
Expand All @@ -355,8 +382,12 @@ XmlRpcClient::readHeader()
return setupConnection();
}

XmlRpcUtil::error("Error in XmlRpcClient::readHeader: error while reading header (%s) on fd %d.",
XmlRpcUtil::error("Error in XmlRpcClient::readHeader: error while reading "
"header (%s) on fd %d.",
XmlRpcSocket::getErrorMsg().c_str(), getfd());
// Read failed; this means the socket is in an unrecoverable state.
// Close the socket.
close();
return false;
}

Expand All @@ -381,6 +412,7 @@ XmlRpcClient::readHeader()
if (_eof) // EOF in the middle of a response is an error
{
XmlRpcUtil::error("Error in XmlRpcClient::readHeader: EOF while reading header");
close();
return false; // Close the connection
}

Expand All @@ -390,12 +422,16 @@ XmlRpcClient::readHeader()
// Decode content length
if (lp == 0) {
XmlRpcUtil::error("Error XmlRpcClient::readHeader: No Content-length specified");
// Close the socket because we can't make further use of it.
close();
return false; // We could try to figure it out by parsing as we read, but for now...
}

_contentLength = atoi(lp);
if (_contentLength <= 0) {
XmlRpcUtil::error("Error in XmlRpcClient::readHeader: Invalid Content-length specified (%d).", _contentLength);
// Close the socket because we can't make further use of it.
close();
return false;
}

Expand All @@ -414,15 +450,23 @@ XmlRpcClient::readResponse()
{
// If we dont have the entire response yet, read available data
if (int(_response.length()) < _contentLength) {
if ( ! XmlRpcSocket::nbRead(this->getfd(), _response, &_eof)) {
std::string buff;
if ( ! XmlRpcSocket::nbRead(this->getfd(), buff, &_eof)) {
XmlRpcUtil::error("Error in XmlRpcClient::readResponse: read error (%s).",XmlRpcSocket::getErrorMsg().c_str());
// nbRead returned an error, indicating that the socket is in a bad state.
// close it and stop monitoring this client.
close();
return false;
}
_response += buff;

// If we haven't gotten the entire _response yet, return (keep reading)
if (int(_response.length()) < _contentLength) {
if (_eof) {
XmlRpcUtil::error("Error in XmlRpcClient::readResponse: EOF while reading response");
// nbRead returned an eof, indicating that the socket is disconnected.
// close it and stop monitoring this client.
close();
return false;
}
return true;
Expand Down
14 changes: 14 additions & 0 deletions utilities/xmlrpcpp/test/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,20 @@ if(TARGET xmlrpcvalue_base64)
target_link_libraries(xmlrpcvalue_base64 xmlrpcpp)
endif()

add_library(mock_socket mock_socket.cpp)

catkin_add_gtest(test_client
test_client.cpp
../src/XmlRpcClient.cpp
../src/XmlRpcValue.cpp
../src/XmlRpcUtil.cpp
../src/XmlRpcDispatch.cpp
../src/XmlRpcSource.cpp
../libb64/src/cdecode.c
../libb64/src/cencode.c
)
target_link_libraries(test_client mock_socket)

if(NOT WIN32)
catkin_add_gtest(test_socket
test_socket.cpp
Expand Down
Loading