[PATCH] apr_pollset_poll() returns WSAEINVAL if no sockets are in the pollset

[Justin Erenkrantz <ju...@erenkrantz.com>]

The patch below fixes a cosmetic problem seen with serf on Win32 if
apr_pollset_poll is called without any sockets already in the pollset.
 select on Win32 will immediately return WSAEINVAL (730022) in this
case.

The MSDN docs for select() -
http://msdn.microsoft.com/en-us/library/ms740141.aspx - says:

---
Any two of the parameters, readfds, writefds, or exceptfds, can be
given as null. At least one must be non-null, and any non-null
descriptor set must contain at least one handle to a socket.
---

Earlier in the doc, it says that select() ignores the first parameter
(nfds), so it looks like while other OSes may have a short-circuit
success on the 0 nfds case, Win32 will just return WSAEINVAL in this
case.  Instead of returning an error, I believe the right thing is for
APR to hide this and return success.

So, any objections to committing the following?  (If we're going to do
1.3.5, I'd like to see this backported too.)

Thanks!  -- justin

Index: poll/unix/select.c
===================================================================
--- poll/unix/select.c  (revision 689358)
+++ poll/unix/select.c  (working copy)
@@ -453,6 +453,17 @@ APR_DECLARE(apr_status_t) apr_pollset_poll(apr_pol
     fd_set readset, writeset, exceptset;
     apr_status_t rv = APR_SUCCESS;

+#ifdef WIN32
+    /* On Win32, select() must be presented with at least one socket to
+     * poll on, or select() will return WSAEINVAL.  So, we'll just
+     * short-circuit and bail now.
+     */
+    if (pollset->nelts == 0) {
+        (*num) = 0;
+        return APR_SUCCESS;
+    }
+#endif
+
     if (timeout < 0) {
         tvptr = NULL;
     }

Re: [PATCH] apr_pollset_poll() returns WSAEINVAL if no sockets are in the pollset

[Stefan Ruppert <ml...@ruppert-it.de>]

Hello Justin,

I think your fix is not correct. In some of my applications I crossed 
exactly this situation and my main event loop took 100% CPU time because 
of this returned error. I use an event loop like the following one 
(running is modified by another thread which causes the event loop to 
terminate):

volatile apr_int32_t running = 1;
void my_event_loop()
{
    apr_interval_time_t timeout =  1000000; /* one second
    const apr_pollfd_t *desc;

    apr_int32_t num=0;
    apr_status_t rc;
    do {
       rc = apr_pollset_poll(mypollset, timeout, &num, &desc);
       if(APR_STATUS_IS_EINVAL(rc)) {
          apr_sleep(timeout);
          num=0;
          rc = APR_TIMEUP;
       }
    } while(running);
}

Note the if-statement with APR_STATUS_IS_EINVAL() check. I think this 
has to go into the APR source apr_pollset_poll() for Windows. Otherwise 
this eventloop will do a busy looping and consume 100% CPU time of the 
current thread!

Also I think the EINVAL return code should be returned if the timeout 
value is INFINITE!?

Any comments?

Regards,
Stefan

Justin Erenkrantz wrote:
> The patch below fixes a cosmetic problem seen with serf on Win32 if
> apr_pollset_poll is called without any sockets already in the pollset.
>  select on Win32 will immediately return WSAEINVAL (730022) in this
> case.
> 
> The MSDN docs for select() -
> http://msdn.microsoft.com/en-us/library/ms740141.aspx - says:
> 
> ---
> Any two of the parameters, readfds, writefds, or exceptfds, can be
> given as null. At least one must be non-null, and any non-null
> descriptor set must contain at least one handle to a socket.
> ---
> 
> Earlier in the doc, it says that select() ignores the first parameter
> (nfds), so it looks like while other OSes may have a short-circuit
> success on the 0 nfds case, Win32 will just return WSAEINVAL in this
> case.  Instead of returning an error, I believe the right thing is for
> APR to hide this and return success.
> 
> So, any objections to committing the following?  (If we're going to do
> 1.3.5, I'd like to see this backported too.)
> 
> Thanks!  -- justin
> 
> Index: poll/unix/select.c
> ===================================================================
> --- poll/unix/select.c  (revision 689358)
> +++ poll/unix/select.c  (working copy)
> @@ -453,6 +453,17 @@ APR_DECLARE(apr_status_t) apr_pollset_poll(apr_pol
>      fd_set readset, writeset, exceptset;
>      apr_status_t rv = APR_SUCCESS;
> 
> +#ifdef WIN32
> +    /* On Win32, select() must be presented with at least one socket to
> +     * poll on, or select() will return WSAEINVAL.  So, we'll just
> +     * short-circuit and bail now.
> +     */
> +    if (pollset->nelts == 0) {
> +        (*num) = 0;
> +        return APR_SUCCESS;
> +    }
> +#endif
> +
>      if (timeout < 0) {
>          tvptr = NULL;
>      }
> 
> 

Re: [PATCH] apr_pollset_poll() returns WSAEINVAL if no sockets are in the pollset

[Mladen Turk <mt...@apache.org>]

Justin Erenkrantz wrote:
> 
> So, any objections to committing the following?  (If we're going to do
> 1.3.5, I'd like to see this backported too.)
>

For trunk you might add extra check for wakeup socket if
pollset was created with APR_POLLSET_WAKEABLE in which case
there is at least one socket in the pollset, but this one
is used only for interrupt operation.
In that case the check should be (pollset->nelts < 2)

Regards
-- 
^(TM)

Re: [PATCH] apr_pollset_poll() returns WSAEINVAL if no sockets are in the pollset

["William A. Rowe, Jr." <wr...@rowe-clan.net>]

Justin Erenkrantz wrote:
> So, any objections to committing the following?  (If we're going to do
> 1.3.5, I'd like to see this backported too.)

+1 to apply to trunk and 1.3 branch, commit away.