From a40aa0237d6ef1be4c333260f71403ac931008bc Mon Sep 17 00:00:00 2001 From: Fabian Keil Date: Thu, 25 May 2017 11:16:56 +0000 Subject: [PATCH] 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 --- jbsockets.c | 65 +++++++++++++++++++++++++++++++++++++++++++++++-- jcc.c | 69 ++++++++++++++++++++++++++++++++++++++++++++++++++--- loadcfg.c | 7 ++++-- 3 files changed, 134 insertions(+), 7 deletions(-) diff --git a/jbsockets.c b/jbsockets.c index 187fbc4d..6a89f683 100644 --- 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 6bcd82e7..bfbde007 100644 --- 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 # endif +#ifdef HAVE_POLL +#ifdef __GLIBC__ +#include +#else +#include +#endif /* def __GLIBC__ */ +#else # ifndef FD_ZERO # include # 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 dc711fde..9625e9b0 100644 --- 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) -- 2.39.2