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;
>      }
> 
>