Never use select() when poll() is available

On most platforms select() is limitted by FD_SETSIZE while
poll() is not. This was a scaling issue for multi-user setups.

Using poll() has no downside other than the usual risk
that code modifications may introduce new bugs that have
yet to be found and fixed.

At least in theory this commit could also reduce the latency
when there are lots of connections and select() would use
"bit fields in arrays of integers" to store file descriptors.

Another side effect is that Privoxy no longer has to stop
monitoring the client sockets when pipelined requests are
waiting but can't be read yet.

This code keeps the select()-based code behind ifdefs for
now but hopefully it can be removed soonish to make the
code more readable.

Sponsored by: Robert Klemme

diff --git a/jbsockets.c b/jbsockets.c
index 187fbc4..6a89f68 100644 (file)
--- a/jbsockets.c
+++ b/jbsockets.c
@@ -1,4 +1,4 @@
-const char jbsockets_rcs[] = "$Id: jbsockets.c,v 1.138 2016/09/27 22:48:28 ler762 Exp $";
+const char jbsockets_rcs[] = "$Id: jbsockets.c,v 1.139 2016/12/24 16:00:49 fabiankeil Exp $";
 /*********************************************************************
  *
  * File        :  $Source: /cvsroot/ijbswa/current/jbsockets.c,v $
@@ -211,8 +211,12 @@ static jb_socket rfc2553_connect_to(const char *host, int portnum, struct client
    char service[6];
    int retval;
    jb_socket fd;
+#ifdef HAVE_POLL
+   struct pollfd poll_fd[1];
+#else
    fd_set wfds;
    struct timeval timeout;
+#endif
 #if !defined(_WIN32) && !defined(__BEOS__) && !defined(AMIGA) && !defined(__OS2__)
    int   flags;
 #endif
@@ -300,6 +304,7 @@ static jb_socket rfc2553_connect_to(const char *host, int portnum, struct client
          continue;
       }
 
+#ifndef HAVE_POLL
 #ifndef _WIN32
       if (fd >= FD_SETSIZE)
       {
@@ -311,6 +316,7 @@ static jb_socket rfc2553_connect_to(const char *host, int portnum, struct client
          return JB_INVALID_SOCKET;
       }
 #endif
+#endif
 
 #ifdef FEATURE_EXTERNAL_FILTERS
       mark_socket_for_close_on_execute(fd);
@@ -363,6 +369,12 @@ static jb_socket rfc2553_connect_to(const char *host, int portnum, struct client
       }
 #endif /* !defined(_WIN32) && !defined(__BEOS__) && !defined(AMIGA) && !defined(__OS2__) */
 
+#ifdef HAVE_POLL
+   poll_fd[0].fd = fd;
+   poll_fd[0].events = POLLOUT;
+
+   if (poll(poll_fd, 1, 30000) > 0)
+#else
       /* wait for connection to complete */
       FD_ZERO(&wfds);
       FD_SET(fd, &wfds);
@@ -373,6 +385,7 @@ static jb_socket rfc2553_connect_to(const char *host, int portnum, struct client
       /* MS Windows uses int, not SOCKET, for the 1st arg of select(). Weird! */
       if ((select((int)fd + 1, NULL, &wfds, NULL, &timeout) > 0)
          && FD_ISSET(fd, &wfds))
+#endif
       {
          socklen_t optlen = sizeof(socket_error);
          if (!getsockopt(fd, SOL_SOCKET, SO_ERROR, &socket_error, &optlen))
@@ -430,8 +443,12 @@ static jb_socket no_rfc2553_connect_to(const char *host, int portnum, struct cli
    struct sockaddr_in inaddr;
    jb_socket fd;
    unsigned int addr;
+#ifdef HAVE_POLL
+   struct pollfd poll_fd[1];
+#else
    fd_set wfds;
    struct timeval tv[1];
+#endif
 #if !defined(_WIN32) && !defined(__BEOS__) && !defined(AMIGA) && !defined(__OS2__)
    int   flags;
 #endif
@@ -493,6 +510,7 @@ static jb_socket no_rfc2553_connect_to(const char *host, int portnum, struct cli
       return(JB_INVALID_SOCKET);
    }
 
+#ifndef HAVE_POLL
 #ifndef _WIN32
    if (fd >= FD_SETSIZE)
    {
@@ -502,6 +520,7 @@ static jb_socket no_rfc2553_connect_to(const char *host, int portnum, struct cli
       close_socket(fd);
       return JB_INVALID_SOCKET;
    }
+#endif
 #endif
 
    set_no_delay_flag(fd);
@@ -549,6 +568,12 @@ static jb_socket no_rfc2553_connect_to(const char *host, int portnum, struct cli
    }
 #endif /* !defined(_WIN32) && !defined(__BEOS__) && !defined(AMIGA) && !defined(__OS2__) */
 
+#ifdef HAVE_POLL
+   poll_fd[0].fd = fd;
+   poll_fd[0].events = POLLOUT;
+
+   if (poll(poll_fd, 1, 30000) <= 0)
+#else
    /* wait for connection to complete */
    FD_ZERO(&wfds);
    FD_SET(fd, &wfds);
@@ -558,6 +583,7 @@ static jb_socket no_rfc2553_connect_to(const char *host, int portnum, struct cli
 
    /* MS Windows uses int, not SOCKET, for the 1st arg of select(). Weird! */
    if (select((int)fd + 1, NULL, &wfds, NULL, tv) <= 0)
+#endif
    {
       close_socket(fd);
       return(JB_INVALID_SOCKET);
@@ -703,10 +729,18 @@ int read_socket(jb_socket fd, char *buf, int len)
  *********************************************************************/
 int data_is_available(jb_socket fd, int seconds_to_wait)
 {
+   int n;
    char buf[10];
+#ifdef HAVE_POLL
+   struct pollfd poll_fd[1];
+
+   poll_fd[0].fd = fd;
+   poll_fd[0].events = POLLIN;
+
+   n = poll(poll_fd, 1, seconds_to_wait * 1000);
+#else
    fd_set rfds;
    struct timeval timeout;
-   int n;
 
    memset(&timeout, 0, sizeof(timeout));
    timeout.tv_sec = seconds_to_wait;
@@ -720,6 +754,7 @@ int data_is_available(jb_socket fd, int seconds_to_wait)
    FD_SET(fd, &rfds);
 
    n = select(fd+1, &rfds, NULL, NULL, &timeout);
+#endif
 
    /*
     * XXX: Do we care about the different error conditions?
@@ -1228,25 +1263,41 @@ int accept_connection(struct client_state * csp, jb_socket fds[])
    int retval;
    int i;
    int max_selected_socket;
+#ifdef HAVE_POLL
+   struct pollfd poll_fds[MAX_LISTENING_SOCKETS];
+   nfds_t polled_sockets;
+#else
    fd_set selected_fds;
+#endif
    jb_socket fd;
    const char *host_addr;
    size_t listen_addr_size;
 
    c_length = sizeof(client);
 
+#ifdef HAVE_POLL
+   memset(poll_fds, 0, sizeof(poll_fds));
+   polled_sockets = 0;
+#else
    /*
     * Wait for a connection on any socket.
     * Return immediately if no socket is listening.
     * XXX: Why not treat this as fatal error?
     */
    FD_ZERO(&selected_fds);
+#endif
    max_selected_socket = 0;
    for (i = 0; i < MAX_LISTENING_SOCKETS; i++)
    {
       if (JB_INVALID_SOCKET != fds[i])
       {
+#ifdef HAVE_POLL
+         poll_fds[i].fd = fds[i];
+         poll_fds[i].events = POLLIN;
+         polled_sockets++;
+#else
          FD_SET(fds[i], &selected_fds);
+#endif
          if (max_selected_socket < fds[i] + 1)
          {
             max_selected_socket = fds[i] + 1;
@@ -1259,7 +1310,11 @@ int accept_connection(struct client_state * csp, jb_socket fds[])
    }
    do
    {
+#ifdef HAVE_POLL
+      retval = poll(poll_fds, polled_sockets, -1);
+#else
       retval = select(max_selected_socket, &selected_fds, NULL, NULL, NULL);
+#endif
    } while (retval < 0 && errno == EINTR);
    if (retval <= 0)
    {
@@ -1277,8 +1332,12 @@ int accept_connection(struct client_state * csp, jb_socket fds[])
       }
       return 0;
    }
+#ifdef HAVE_POLL
+   for (i = 0; i < MAX_LISTENING_SOCKETS && (poll_fds[i].revents == 0); i++);
+#else
    for (i = 0; i < MAX_LISTENING_SOCKETS && !FD_ISSET(fds[i], &selected_fds);
          i++);
+#endif
    if (i >= MAX_LISTENING_SOCKETS)
    {
       log_error(LOG_LEVEL_ERROR,
@@ -1325,6 +1384,7 @@ int accept_connection(struct client_state * csp, jb_socket fds[])
    }
 #endif
 
+#ifndef HAVE_POLL
 #ifndef _WIN32
    if (afd >= FD_SETSIZE)
    {
@@ -1335,6 +1395,7 @@ int accept_connection(struct client_state * csp, jb_socket fds[])
       return 0;
    }
 #endif
+#endif
 
 #ifdef FEATURE_EXTERNAL_FILTERS
    mark_socket_for_close_on_execute(afd);

diff --git a/jcc.c b/jcc.c
index 6bcd82e..bfbde00 100644 (file)
--- a/jcc.c
+++ b/jcc.c
@@ -1,4 +1,4 @@
-const char jcc_rcs[] = "$Id: jcc.c,v 1.454 2017/05/25 11:14:38 fabiankeil Exp $";
+const char jcc_rcs[] = "$Id: jcc.c,v 1.455 2017/05/25 11:16:04 fabiankeil Exp $";
 /*********************************************************************
  *
  * File        :  $Source: /cvsroot/ijbswa/current/jcc.c,v $
@@ -95,9 +95,17 @@ const char jcc_rcs[] = "$Id: jcc.c,v 1.454 2017/05/25 11:14:38 fabiankeil Exp $"
 # include <os2.h>
 # endif
 
+#ifdef HAVE_POLL
+#ifdef __GLIBC__
+#include <sys/poll.h>
+#else
+#include <poll.h>
+#endif /* def __GLIBC__ */
+#else
 # ifndef FD_ZERO
 #  include <select.h>
 # endif
+#endif /* HAVE_POLL */
 
 #endif
 
@@ -1957,9 +1965,14 @@ static void handle_established_connection(struct client_state *csp,
    char buf[BUFFER_SIZE];
    char *hdr;
    char *p;
-   fd_set rfds;
    int n;
+#ifdef HAVE_POLL
+   struct pollfd poll_fds[2];
+#else
+   fd_set rfds;
    jb_socket maxfd;
+   struct timeval timeout;
+#endif
    int server_body;
    int ms_iis5_hack = 0;
    unsigned long long byte_count = 0;
@@ -1969,7 +1982,6 @@ static void handle_established_connection(struct client_state *csp,
 
    /* Skeleton for HTTP response, if we should intercept the request */
    struct http_response *rsp;
-   struct timeval timeout;
 #ifdef FEATURE_CONNECTION_KEEP_ALIVE
    int watch_client_socket;
 #endif
@@ -1978,8 +1990,10 @@ static void handle_established_connection(struct client_state *csp,
 
    http = csp->http;
 
+#ifndef HAVE_POLL
    maxfd = (csp->cfd > csp->server_connection.sfd) ?
       csp->cfd : csp->server_connection.sfd;
+#endif
 
    /* pass data between the client and server
     * until one or the other shuts down the connection.
@@ -1993,6 +2007,7 @@ static void handle_established_connection(struct client_state *csp,
 
    for (;;)
    {
+#ifndef HAVE_POLL
 #ifdef __OS2__
       /*
        * FD_ZERO here seems to point to an errant macro which crashes.
@@ -2014,6 +2029,7 @@ static void handle_established_connection(struct client_state *csp,
       }
 
       FD_SET(csp->server_connection.sfd, &rfds);
+#endif /* ndef HAVE_POLL */
 
 #ifdef FEATURE_CONNECTION_KEEP_ALIVE
       if ((csp->flags & CSP_FLAG_CHUNKED)
@@ -2058,9 +2074,32 @@ static void handle_established_connection(struct client_state *csp,
       }
 #endif  /* FEATURE_CONNECTION_KEEP_ALIVE */
 
+#ifdef HAVE_POLL
+      poll_fds[0].fd = csp->cfd;
+#ifdef FEATURE_CONNECTION_KEEP_ALIVE
+      if (!watch_client_socket)
+      {
+         /*
+          * Ignore incoming data, but still watch out
+          * for disconnects etc. These flags are always
+          * implied anyway but explicitly setting them
+          * doesn't hurt.
+          */
+         poll_fds[0].events = POLLERR|POLLHUP;
+      }
+      else
+#endif
+      {
+         poll_fds[0].events = POLLIN;
+      }
+      poll_fds[1].fd = csp->server_connection.sfd;
+      poll_fds[1].events = POLLIN;
+      n = poll(poll_fds, 2, csp->config->socket_timeout * 1000);
+#else
       timeout.tv_sec = csp->config->socket_timeout;
       timeout.tv_usec = 0;
       n = select((int)maxfd+1, &rfds, NULL, NULL, &timeout);
+#endif /* def HAVE_POLL */
 
       if (n == 0)
       {
@@ -2075,7 +2114,11 @@ static void handle_established_connection(struct client_state *csp,
       }
       else if (n < 0)
       {
+#ifdef HAVE_POLL
+         log_error(LOG_LEVEL_ERROR, "poll() failed!: %E");
+#else
          log_error(LOG_LEVEL_ERROR, "select() failed!: %E");
+#endif
          mark_server_socket_tainted(csp);
          return;
       }
@@ -2087,7 +2130,21 @@ static void handle_established_connection(struct client_state *csp,
        * XXX: Make sure the client doesn't use pipelining
        * behind Privoxy's back.
        */
+#ifdef HAVE_POLL
+      if ((poll_fds[0].revents & (POLLERR|POLLHUP|POLLNVAL)) != 0)
+      {
+         log_error(LOG_LEVEL_CONNECT,
+            "The client socket %d has become unusable while "
+            "the server socket %d is still open.",
+            csp->cfd, csp->server_connection.sfd);
+         mark_server_socket_tainted(csp);
+         break;
+      }
+
+      if (poll_fds[0].revents != 0)
+#else
       if (FD_ISSET(csp->cfd, &rfds))
+#endif /* def HAVE_POLL*/
       {
          int max_bytes_to_read = sizeof(buf) - 1;
 
@@ -2175,7 +2232,11 @@ static void handle_established_connection(struct client_state *csp,
        * If `hdr' is null, then it's the header otherwise it's the body.
        * FIXME: Does `hdr' really mean `host'? No.
        */
+#ifdef HAVE_POLL
+      if (poll_fds[1].revents != 0)
+#else
       if (FD_ISSET(csp->server_connection.sfd, &rfds))
+#endif /* HAVE_POLL */
       {
 #ifdef FEATURE_CONNECTION_KEEP_ALIVE
          /*
@@ -4048,6 +4109,7 @@ static jb_socket bind_port_helper(const char *haddr, int hport)
       return JB_INVALID_SOCKET;
    }
 
+#ifndef HAVE_POLL
 #ifndef _WIN32
    if (bfd >= FD_SETSIZE)
    {
@@ -4055,6 +4117,7 @@ static jb_socket bind_port_helper(const char *haddr, int hport)
          "Bind socket number too high to use select(): %d >= %d",
          bfd, FD_SETSIZE);
    }
+#endif
 #endif
 
    if (haddr == NULL)

diff --git a/loadcfg.c b/loadcfg.c
index dc711fd..9625e9b 100644 (file)
--- a/loadcfg.c
+++ b/loadcfg.c
@@ -1,4 +1,4 @@
-const char loadcfg_rcs[] = "$Id: loadcfg.c,v 1.156 2017/02/24 11:59:44 fabiankeil Exp $";
+const char loadcfg_rcs[] = "$Id: loadcfg.c,v 1.157 2017/05/20 09:24:35 fabiankeil Exp $";
 /*********************************************************************
  *
  * File        :  $Source: /cvsroot/ijbswa/current/loadcfg.c,v $
@@ -1375,7 +1375,7 @@ struct configuration_spec * load_config(void)
          {
             int max_client_connections = parse_numeric_value(cmd, arg);
 
-#ifndef _WIN32
+#if !defined(_WIN32) && !defined(HAVE_POLL)
             /*
              * Reject values below 1 for obvious reasons and values above
              * FD_SETSIZE/2 because Privoxy needs two sockets to serve
@@ -1400,6 +1400,9 @@ struct configuration_spec * load_config(void)
              * passed to select().
              * https://msdn.microsoft.com/en-us/library/windows/desktop/ms739169%28v=vs.85%29.aspx
              *
+             * On platforms were we use poll() we don't have to enforce
+             * an upper connection limit either.
+             *
              * XXX: Do OS/2, Amiga etc. belong here as well?
              */
             if (max_client_connections < 1)