Synchronous client connection (with no timeout set) to non listening socket blocks indefinitely [SOLVED]

[fammollo]

Hello,

I am testing on Windows 7 the C++ library redis3m, based upon hiredis and boost, and I am using the hiredis client/Win32_Interop libraries (git tag 2.8.17.3) provided by you.

I have compiled from scratch all the needed components, both hiredis and redis3m along with all its dependencies, with Visual Studio Express 2013 for Windows Desktop, the only change I have made to your project files is to use the DLL version of the C/C++ runtime libraries.
The resulting executable uses only static libraries, I have no additional dependencies upon DLLs exept for those provided by Windows itself; Dependency Walker shows for the debug version: KERNEL32.DLL, WS2_32.DLL, MSVCP120D.DLL, MSVCR120D.DLL, USER32.DLL, ADVAPI32.DLL.

I am writing you here because I have experienced this issue: it seems that a blocking connection to redis with no timeout set, initiated by calling redisConnect (host, port) blocks indefinitely if the remote port is not open, I would have expected instead returning from the function and c->err set to an appropriate error.

Stepping inside the library code, I saw that inside the function _redisContextConnectTcp:

• connect (redis_connect_impl, actually) is called and it returns EINPROGRESS because WSAGetLastError returns 10035 (WSAEWOULDBLOCK)

• After that redisContextWaitReady is called, and the execution blocks indefinitely inside it, specifically when calling poll on the socket:

  if (errno == EINPROGRESS) {
  int res;

  if ((res = poll(wfd, 1, msec)) == -1) {
  

After reading here: https://msdn.microsoft.com/en-us/library/windows/desktop/ms741669%28v=vs.85%29.aspx

and here: http://curl.haxx.se/mail/lib-2012-10/0038.html

At first I tried changing the polled events from just POLLOUT to POLLOUT | POLLERR, but then a legitimate connection did not work anymore (I didn't investigate this), so in the end I chose to try to use select instead of poll (keeping POLLERR, so that internally the socket is added to the excepSet).

I opted to enable using poll only if the OS is at least Windows 8 by changing this check:

if (IsWindowsVersionAtLeast(HIBYTE(_WIN32_WINNT_WIN6), LOBYTE(_WIN32_WINNT_WIN6), 0)) {

to this one:

IsWindowsVersionAtLeast(HIBYTE(0x0602), LOBYTE(0x0602), 0)) {

(the SDK provided by VS 2003 Express does not now anything about the _WIN8/_WINBLUE defines)

but there were again some problems (caused, I think, by the code not being really maintained/tested) so in the end I debugged and modified the code and I finally got it working.

I don't know if using select already works in the 2.8 branch (or in some other version); if it isn't, I would like to contribute my modification to you (if it's appreciated).

Thank you in advance and best regards
Fabrizio

[orangemocha]

Hi @fammollo . Your analysis seems correct, and WSAPoll is hanging even on my Windows 8.1 machine. It's very easy to reproduce, by just running redis-cli with no server running.
So yes, if you made the select path working, a pull request would be very much appreciated! I think we should make it the default path regardless of Windows version. I would leave the WSAPoll path under #ifdef USE_WSAPOLL.
Thanks!!

[orangemocha]

@fammollo do you still have the changes that solved this issue? A PR would definitely be welcome 😄

[fammollo]

Hello,

I’m sorry, I haven’t been able yet to contribute the patch using github because I still have to actually learn more git and github concepts ☺.

If it is not a problem, here are the changes in unified diff format (generated by TortoiseGitUDiff), I only touched two files. The changes are against redis-win-2.8.17.3.

diff --git a/deps/hiredis/net.c b/deps/hiredis/net.c
index 8a02961..7efecfa 100644
--- a/deps/hiredis/net.c
+++ b/deps/hiredis/net.c
@@ -214,7 +214,7 @@ static int redisContextWaitReady(redisContext *c, const struct timeval *timeout)
msec = -1;
wfd[0].fd = c->fd;

• wfd[0].events = POLLOUT;
• wfd[0].events = POLLOUT | POLLERR;
  /* Only use timeout when not NULL. */
  if (timeout != NULL) {

diff --git a/src/Win32_Interop/Win32_FDAPI.cpp b/src/Win32_Interop/Win32_FDAPI.cpp
index 6074f86..a007017 100644
--- a/src/Win32_Interop/Win32_FDAPI.cpp
+++ b/src/Win32_Interop/Win32_FDAPI.cpp
@@ -432,21 +432,24 @@ int redis_FD_ISSET_impl(int fd, fd_set* pSet) {
int redis_poll_impl(struct pollfd *fds, nfds_t nfds, int timeout) {
try {

•    struct pollfd\* pollCopy = new struct pollfd[nfds];
  

•    if (pollCopy == NULL) {
  

•        errno = ENOMEM;
  

•        return -1;
  

•    }
  

•    // NOTE: Treating the fds.fd as a Redis file descriptor and converting to a SOCKET for WSAPoll.
  

•    for (nfds_t n = 0; n < nfds; n ++) {
  

•        pollCopy[n].fd = RFDMap::getInstance().lookupSocket((RFD)(fds[n].fd));
  

•        pollCopy[n].events = fds[n].events;
  

•        pollCopy[n].revents = fds[n].revents;
  

•    }
  

•    //if (IsWindowsVersionAtLeast(HIBYTE(_WIN32_WINNT_WIN6), LOBYTE(_WIN32_WINNT_WIN6), 0)) {
  

•                         if (IsWindowsVersionAtLeast(HIBYTE(0x0602), LOBYTE(0x0602), 0)) {
  

•                                        struct pollfd\* pollCopy = new struct pollfd[nfds];
  

•                                        if (pollCopy == NULL) {
  

•                                                        errno = ENOMEM;
  

•                                                        return -1;
  

•                                        }
  

•                                        // NOTE: Treating the fds.fd as a Redis file descriptor and converting to a SOCKET for WSAPoll.
  

•                                        for (nfds_t n = 0; n < nfds; n++) {
  

•                                                        pollCopy[n].fd = RFDMap::getInstance().lookupSocket((RFD)(fds[n].fd));
  

•                                                        pollCopy[n].events = fds[n].events;
  

•                                                        pollCopy[n].revents = fds[n].revents;
  

•                                        }
  

•    if (IsWindowsVersionAtLeast(HIBYTE(_WIN32_WINNT_WIN6), LOBYTE(_WIN32_WINNT_WIN6), 0)) {
  

•        static auto f_WSAPoll = dllfunctor_stdcall<int, WSAPOLLFD*, ULONG, INT>("ws2_32.dll", "WSAPoll");
  

•                                        static auto f_WSAPoll = dllfunctor_stdcall<int, WSAPOLLFD*, ULONG, INT>("ws2_32.dll", "WSAPoll");
       // See the community addition comments at http://msdn.microsoft.com/en-us/library/windows/desktop/ms741669%28v=vs.85%29.aspx for this API.
       // BugCheck seems to indicate that problems with this API in Win8 have been addressed, but this needs to be verified.
  

  @@ -462,7 +465,6 @@ int redis_poll_impl(struct pollfd *fds, nfds_t nfds, int timeout) {
  return ret;
  } else {

•        int ret;
       fd_set readSet;
       fd_set writeSet;
       fd_set excepSet;
  

  @@ -473,6 +475,7 @@ int redis_poll_impl(struct pollfd *fds, nfds_t nfds, int timeout) {
  if (nfds >= FD_SETSIZE) {
  errno = EINVAL;
  +
  return -1;
  }
  @@ -481,16 +484,14 @@ int redis_poll_impl(struct pollfd *fds, nfds_t nfds, int timeout) {
  if (fds[i].fd < 0) {
  continue;
  }

•            if (pollCopy[i].fd >= FD_SETSIZE) {
  

•                errno = EINVAL;
  

•                return -1;
  

•            }
  

•            if (pollCopy[i].events & POLLIN) FD_SET(pollCopy[i].fd, &readSet);
  

•            if (pollCopy[i].events & POLLOUT) FD_SET(pollCopy[i].fd, &writeSet);
  

•            if (pollCopy[i].events & POLLERR) FD_SET(pollCopy[i].fd, &excepSet);
  

•                                                        if (fds[i].events & POLLIN) FD_SET(fds[i].fd, &readSet);
  

•                                                        if (fds[i].events & POLLOUT) FD_SET(fds[i].fd, &writeSet);
  

•                                                        if (fds[i].events & POLLERR) FD_SET(fds[i].fd, &excepSet);
       }
  

•                                        int ret;
  

  •    if (timeout < 0) {
           ret = select(0, &readSet, &writeSet, &excepSet, NULL);
       } else {
    

  @@ -501,20 +502,18 @@ int redis_poll_impl(struct pollfd *fds, nfds_t nfds, int timeout) {
  }
  if (ret < 0) {

•            return ret;
  

•                                                        return ret;
       }
       for (i = 0; i < nfds; i++) {
           fds[i].revents = 0;
  

•            if (FD_ISSET(pollCopy[i].fd, &readSet)) fds[i].revents |= POLLIN;
  

•            if (FD_ISSET(pollCopy[i].fd, &writeSet)) fds[i].revents |= POLLOUT;
  

•            if (FD_ISSET(pollCopy[i].fd, &excepSet)) fds[i].revents |= POLLERR;
  

•                                                        if (FD_ISSET(fds[i].fd, &readSet)) fds[i].revents |= POLLIN;
  

•                                                        if (FD_ISSET(fds[i].fd, &writeSet)) fds[i].revents |= POLLOUT;
  

•                                                        if (FD_ISSET(fds[i].fd, &excepSet)) fds[i].revents |= POLLERR;
       }
  

•        delete pollCopy;
  

- pollCopy = NULL;

         return ret;
     }
 } CATCH_AND_REPORT()

Best regards,
Fabrizio

Da: Alexis Campailla [mailto:notifications@github.com]
Inviato: giovedì 25 giugno 2015 13:47
A: MSOpenTech/redis
Cc: Fabrizio Ammollo
Oggetto: Re: [redis] Synchronous client connection (with no timeout set) to non listening socket blocks indefinitely SOLVED

@fammollohttps://github.com/fammollo do you still have the changes that solved this issue? A PR would definitely be welcome [:smile:]

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/214#issuecomment-115219267.

Informativa Privacy - Ai sensi del D. Lgs n. 196/2003 (Codice Privacy) precisiamo che le informazioni contenute in questo messaggio sono riservate e a uso esclusivo del destinatario. Ogni uso, copia o distribuzione non autorizzata è proibita e passibile di sanzioni ai termini di legge. Reitek non è responsabile di eventuali copie o distribuzioni non autorizzate. Se questo messaggio è stato ricevuto per errore, preghiamo gentilmente di eliminarlo e di informare il mittente. Grazie.

[orangemocha]

Ok, we'll try to figure it out from here.

Grazie mille! 😄

[fammollo]

Hello,

I have finally managed to install the github client and learn something about it ☺, so I have already sent a pull request with the changes I made and signed the license agreement.

Best regards,
Fabrizio

Da: Alexis Campailla [mailto:notifications@github.com]
Inviato: giovedì 25 giugno 2015 16:36
A: MSOpenTech/redis
Cc: Fabrizio Ammollo
Oggetto: Re: [redis] Synchronous client connection (with no timeout set) to non listening socket blocks indefinitely SOLVED

Ok, we'll try to figure it out from here.

Grazie mille! [:smile:]

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/214#issuecomment-115277956.

Informativa Privacy - Ai sensi del D. Lgs n. 196/2003 (Codice Privacy) precisiamo che le informazioni contenute in questo messaggio sono riservate e a uso esclusivo del destinatario. Ogni uso, copia o distribuzione non autorizzata è proibita e passibile di sanzioni ai termini di legge. Reitek non è responsabile di eventuali copie o distribuzioni non autorizzate. Se questo messaggio è stato ricevuto per errore, preghiamo gentilmente di eliminarlo e di informare il mittente. Grazie.

[ShaiWavesAudio]

Using 3.0.5 I'm getting stuck when calling redisConnect and no server is up. Has this issue been fixed like @fammollo says?

I can also see in hiredis.c line 1037:

c->flags |= REDIS_BLOCK; redisContextConnectTcp(c,ip,port,NULL);

It seems that the REDIS_BLOACK bit is hard coded. What gives?

Thanks
Shai

[enricogior]

Hi @ShaiWavesAudio
this bug hasn't been fixed and unfortunately it won't be fixed since it's not an application bug but a system API bug and the WIndows team has decided that they won't fix it.
REDIS_BLOCK is unrelated to this bug.

[ShaiWavesAudio]

I understand the difficulty of overcoming a system API bug. However isn't the whole idea of MSOpenTech/redis to adjust the redis code to Windows, including Windows' bugs and limitations? As it is now I cannot ship my redis support to Windows users because if the redis server is not up, the whole process will hang. I'm willing to put work to overcome this bug, but need pointer on where to start.
Thanks,
Shai

[enricogior]

@ShaiWavesAudio
you can use a workaround and disable the bugged API in Win32_FDAPI.cpp::FDAPI_poll().
Change if (WindowsVersion::getInstance().IsAtLeast_6_0()) to if(FALSE).
Let me know if that helped.

[ShaiWavesAudio]

Thanks @enricogior.
Changing to if(FALSE) prevented the code getting hanged when the redis server was not available. However, when redis server is available the connection fails. I traced the fail to the lines:

if (pollCopy[i].fd >= FD_SETSIZE) { errno = EINVAL; return -1; }
Where pollCopy[i].fd == 1032 but FD_SETSIZE == 64.

To overcome that I redefined FD_SETSIZE to 2084. Now the fail is in the line:

ret = select(0, &readSet, &writeSet, &excepSet, NULL);

select returned -1 and so connection failed.

Any idea how to continue?

Again thanks for the help...

Shai

[enricogior]

@ShaiWavesAudio
change line 635 from:
pollCopy[n].fd = RFDMap::getInstance().lookupSocket((RFD) (fds[n].fd));
to:
pollCopy[n].fd = fds[n].fd;

You can also remove if (pollCopy[i].fd >= FD_SETSIZE) { errno = EINVAL; return -1; } since that check is completely wrong.

Let me know if it works for you.
Thank you.

[fammollo]

Hello,

I have just "come back" to redis-related development, and, after downloading the latest (3.2-100) port, I noticed that this issue is still there... I ported my old changes (with just a couple of minor adjustments) and now the behaviour is again the correct one.

I am proposing to submit my patch again, if it can be useful to anybody.

I am only using a Windows 7 machine right now, so I don't know if the poll issue is actually solved on later Windows versions, what is described into the comment made my enricogior on last 9 May is a possible solution to forget about WSAPoll.

Best regards,
Fabrizio

[fammollo]

"what is described into the comment made my enricogior on last 9 May is a possible solution to forget about WSAPoll"

I meant to write this but also adding " but the code as it is does not work with select, my patch makes it work too in that case".