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

[Windows][melodic-devel] workaround WSAPoll doesn't report failed connections. #1816

Merged
merged 2 commits into from
Feb 3, 2020
Merged

[Windows][melodic-devel] workaround WSAPoll doesn't report failed connections. #1816

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6e333f8
workaround wsapoll not report failed connections.
Sep 26, 2019
8196eb0
add link for reference.
seanyen Jan 9, 2020
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
52 changes: 50 additions & 2 deletions utilities/xmlrpcpp/src/XmlRpcDispatch.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,57 @@

#if defined(_WINDOWS)
# include <winsock2.h>
static inline int poll( struct pollfd *pfd, int nfds, int timeout)
static int poll( struct pollfd *pfd, int nfds, int timeout)
{
return WSAPoll(pfd, nfds, timeout);
// workaround: "Windows 8 Bugs 309411 – WSAPoll does not report failed connections"
// https://curl.haxx.se/mail/lib-2012-10/0038.html
// the following logic is to use select() to check all writable socket connnection status.
// if all the sockets to be checked are not connected, it reports SOCKET_ERROR to
// error out, instead of going to WSAPoll() which causes infinitely wait situation.
FD_SET writable;
FD_SET error;
FD_ZERO(&writable);
FD_ZERO(&error);
for (int i = 0; i < nfds; ++i)
{
if (pfd[i].events & POLLOUT)
{
FD_SET(pfd[i].fd, &writable);
FD_SET(pfd[i].fd, &error);
}
}

int connectionError = 0;
if (writable.fd_count > 0)
{
int result = select(0, nullptr, &writable, &error, nullptr);
if (SOCKET_ERROR == result)
{
return SOCKET_ERROR;
}

if (0 != result)
{
for (int i = 0; i < nfds; ++i)
{
if ((pfd[i].events & POLLOUT) &&
(FD_ISSET(pfd[i].fd, &error)))
{
connectionError++;
}
}
}
}

if (connectionError == nfds)
{
// error out if all sockets are failed to connect.
return SOCKET_ERROR;
}
else
{
return WSAPoll(pfd, nfds, timeout);
}
}

# define USE_FTIME
Copy link
Member

@mikepurvis mikepurvis Jan 15, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I know it's not directly related here, but in the process of reviewing this, I noticed some if/endif mismatching going on. The #endif at the bottom of the file is labeled for USE_FTIME but I believe actually corresponds to WINDOWS.

@seanyen Do you mind looking into this and correcting the comments if applicable?

Copy link
Contributor Author

@seanyen seanyen Jan 15, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

By checking the commit history, it seems to me that USE_FTIME was first introduced because Windows doesn't have gettimeofday so it ended up with diverged implementations. However, two years ago, a PR was merged and now non-Windows implementation is in favor of ros::ros_steadytime.

I would say perhaps we should see if there is any opportunity to converge the implementations now, since ros::ros_steadytime is a portable thing and theoretically it should behave the same across the platforms.

@mikepurvis, what do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Since this is orthogonal please follow up on this separately form this PR.

Expand Down