Unit tests for XmlRpcDispatch #1232
Conversation
trainman419
commented
Nov 13, 2017
trainman419
commented
- Fix handling of out-of-band (Exception) request and and event processing to match previous select() implementation.
- Update comments to describe the events that actually trigger and Exception state.
- Unit tests for XmlRpcDispatch to verify that it calls poll() correctly, requests the correct events and calls the correct handler for those events.
* Fix handling of out-of-band (Exception) request and and event processing to match previous select() implementation. * Update comments to describe the events that actually trigger and Exception state. * Unit tests for XmlRpcDispatch to verify that it calls poll() correctly, requests the correct events and calls the correct handler for those events.
|
Bump. @dirk-thomas @mikepurvis ? |
| 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); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
Fun, okay. Yeah, we're having some similar issues with select/poll/epoll subtleties over in #1217.
|
This all seems reasonable to me, but TBH I've never really had to grapple with these internals before, so my thumbs-up is not the most valuable one. |
There was a problem hiding this comment.
LGTM.
I will wait with the merge until CI is green and @trainman419 had a chance to answer the pending question from @mikepurvis.
|
@ros-pull-request-builder retest this please |
