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 for XmlRpcDispatch #1232

Merged
merged 3 commits into from
Nov 16, 2017
Merged

Unit tests for XmlRpcDispatch #1232

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8d40344
Unit tests for XmlRpcDispatch
trainman419 Nov 13, 2017
6ce5a53
Cleanup from internal review
trainman419 Nov 13, 2017
ec26e04
revert unnecessary whitespace changes
dirk-thomas Nov 16, 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
2 changes: 1 addition & 1 deletion utilities/xmlrpcpp/include/xmlrpcpp/XmlRpcDispatch.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ namespace XmlRpc {
enum EventType {
ReadableEvent = 1, //!< data available to read
WritableEvent = 2, //!< connected/data can be written without blocking
Exception = 4 //!< uh oh
Exception = 4 //!< out-of-band data has arrived
};

//! Monitor this source for the event types specified by the event mask
Expand Down
11 changes: 7 additions & 4 deletions utilities/xmlrpcpp/src/XmlRpcDispatch.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,8 @@ XmlRpcDispatch::work(double timeout)
const unsigned POLLIN_CHK = (POLLIN | POLLHUP | POLLERR); // Readable or connection lost
const unsigned POLLOUT_REQ = POLLOUT; // Request write
const unsigned POLLOUT_CHK = (POLLOUT | POLLERR); // Writable or connection lost
const unsigned POLLEX_CHK = (POLLPRI | POLLNVAL); // Exception or invalid fd
const unsigned POLLEX_REQ = POLLPRI; // Out-of-band data received
const unsigned POLLEX_CHK = (POLLPRI | POLLNVAL); // Out-of-band data or invalid fd

// Compute end time
_endTime = (timeout < 0.0) ? -1.0 : (getTime() + timeout);
Expand All @@ -110,6 +111,7 @@ XmlRpcDispatch::work(double timeout)
fds[i].events = 0;
if (it->getMask() & ReadableEvent) fds[i].events |= POLLIN_REQ;
if (it->getMask() & WritableEvent) fds[i].events |= POLLOUT_REQ;
if (it->getMask() & Exception) fds[i].events |= POLLEX_REQ;
}

// Check for events
Expand All @@ -132,11 +134,12 @@ XmlRpcDispatch::work(double timeout)
// Only handle requested events to avoid being prematurely removed from dispatch
bool readable = (pfd.events & POLLIN_REQ) == POLLIN_REQ;
bool writable = (pfd.events & POLLOUT_REQ) == POLLOUT_REQ;
bool oob = (pfd.events & POLLEX_REQ) == POLLEX_REQ;
if (readable && (pfd.revents & POLLIN_CHK))
newMask &= src->handleEvent(ReadableEvent);
if (writable && (pfd.revents & POLLOUT_CHK))
newMask &= src->handleEvent(WritableEvent);
if (pfd.revents & POLLEX_CHK)
if (oob && (pfd.revents & POLLEX_CHK))
newMask &= src->handleEvent(Exception);
Copy link
Member

Choose a reason for hiding this comment

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

Can you describe more about what is going on here? Is this a behaviour change/fix which arose as a result of the added tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I initially wrote this test against the previous version of this, which was using select(), since our internal fork of ROS is mostly based on Indigo. When I updated this test to the most recent version of ros_comm to get the poll() implementation, it no longer behaved the same way when there was an exception event on the socket. This fixes it so that the poll() implementation behaves more like the original select() implementation.

Also of note is that for TCP sockets, the exception event actually indicates the reception of out-of-band data over the TCP connection, and doesn't indicate an error event on the socket. I've updated the comments and the implementation to reflect this and restore the original behavior.

(I light of this I think the original behavior might be a little misguided, but my goal here is to preserve all of the existing behavior that isn't clearly a bug.)

Copy link
Member

Choose a reason for hiding this comment

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

Fun, okay. Yeah, we're having some similar issues with select/poll/epoll subtleties over in #1217.


// Find the source iterator. It may have moved as a result of the way
Expand All @@ -156,8 +159,9 @@ XmlRpcDispatch::work(double timeout)

if ( ! newMask) {
_sources.erase(thisIt); // Stop monitoring this one
if ( ! src->getKeepOpen())
if ( ! src->getKeepOpen()) {
src->close();
}
} else if (newMask != (unsigned) -1) {
thisIt->getMask() = newMask;
}
Expand Down Expand Up @@ -227,4 +231,3 @@ XmlRpcDispatch::getTime()
#endif /* USE_FTIME */
}


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 @@ -17,6 +17,20 @@ catkin_add_gtest(test_client
)
target_link_libraries(test_client mock_socket)

catkin_add_gtest(test_dispatch
test_dispatch.cpp
../src/XmlRpcDispatch.cpp
../src/XmlRpcSource.cpp
../src/XmlRpcUtil.cpp
../libb64/src/cdecode.c
../libb64/src/cencode.c
)
target_link_libraries(test_dispatch mock_socket)
set_target_properties(test_dispatch PROPERTIES
LINK_FLAGS
"-Wl,--wrap=select -Wl,--wrap=poll"
)

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