[BUG] fix truncated responses with sepoll

Due to the way Linux delivers EPOLLIN and EPOLLHUP, a closed connection
received after some server data sometimes results in truncated responses
if the client disconnects before server starts to respond. The reason
is that the EPOLLHUP flag is processed as an indication of end of
transfer while some data may remain in the system's socket buffers.

This problem could only be triggered with sepoll, although nothing should
prevent it from happening with normal epoll. In fact, the work factoring
performed by sepoll increases the risk that this bug appears.

The fix consists in making FD_POLL_HUP and FD_POLL_ERR sticky and that
they are only checked if FD_POLL_IN is not set, meaning that we have
read all pending data.

That way, the problem is definitely fixed and sepoll still remains about
17% faster than epoll since it can take into account all information
returned by the kernel.

diff --git a/include/proto/fd.h b/include/proto/fd.h
index 287c8bd80a3555df8c5767c044f98f1469e1d2ff..74b04b29edde78f245b2dc4689d28b84dfb1d8f9 100644 (file)
--- a/include/proto/fd.h
+++ b/include/proto/fd.h
@@ -74,9 +74,10 @@ void run_poller();
 #define EV_FD_CLO(fd)        (cur_poller.clo(fd))
 
 
-/* recomputes the maxfd limit from the fd */
+/* Prepares <fd> for being polled */
 static inline void fd_insert(int fd)
 {
+       fdtab[fd].ev = 0;
        if (fd + 1 > maxfd)
                maxfd = fd + 1;
 }

diff --git a/include/types/fd.h b/include/types/fd.h
index 488887ac094b2348f28231d19c5abb7b797aa49d..1f1b3b4e8d281842190cb997c7d54f5014a860c8 100644 (file)
--- a/include/types/fd.h
+++ b/include/types/fd.h
@@ -45,16 +45,19 @@ enum {
        DIR_SIZE
 };
 
-
+/*
+ * FD_POLL_IN remains set as long as some data is pending for read.
+ * FD_POLL_OUT remains set as long as the fd accepts to write data.
+ * FD_POLL_ERR and FD_POLL_ERR remain set forever (until processed).
+ */
 #define FD_POLL_IN     0x01
 #define FD_POLL_PRI    0x02
 #define FD_POLL_OUT    0x04
 #define FD_POLL_ERR    0x08
 #define FD_POLL_HUP    0x10
-#define FD_POLL_ANY    0x1F
 
-#define FD_POLL_RD     (FD_POLL_IN  | FD_POLL_ERR | FD_POLL_HUP)
-#define FD_POLL_WR     (FD_POLL_OUT | FD_POLL_ERR | FD_POLL_HUP)
+#define FD_POLL_DATA    (FD_POLL_IN  | FD_POLL_OUT)
+#define FD_POLL_STICKY  (FD_POLL_ERR | FD_POLL_HUP)
 
 /* info about one given fd */
 struct fdtab {

diff --git a/src/checks.c b/src/checks.c
index 20ff8db03a3ba8097e2d87848c453ec0cd8a7794..ff02d016fe622dbb01ca310950879bd662767648 100644 (file)
--- a/src/checks.c
+++ b/src/checks.c
@@ -240,12 +240,12 @@ static int event_srv_chk_w(int fd)
        task_wakeup(t);
  out_nowake:
        EV_FD_CLR(fd, DIR_WR);   /* nothing more to write */
-       fdtab[fd].ev &= ~FD_POLL_WR;
+       fdtab[fd].ev &= ~FD_POLL_OUT;
        return 1;
  out_poll:
        /* The connection is still pending. We'll have to poll it
         * before attempting to go further. */
-       fdtab[fd].ev &= ~FD_POLL_WR;
+       fdtab[fd].ev &= ~FD_POLL_OUT;
        return 0;
  out_error:
        s->result |= SRV_CHK_ERROR;
@@ -296,7 +296,7 @@ static int event_srv_chk_r(int fd)
 #endif
        if (unlikely(len < 0 && errno == EAGAIN)) {
                /* we want some polling to happen first */
-               fdtab[fd].ev &= ~FD_POLL_RD;
+               fdtab[fd].ev &= ~FD_POLL_IN;
                return 0;
        }
 
@@ -346,7 +346,7 @@ static int event_srv_chk_r(int fd)
 
        EV_FD_CLR(fd, DIR_RD);
        task_wakeup(t);
-       fdtab[fd].ev &= ~FD_POLL_RD;
+       fdtab[fd].ev &= ~FD_POLL_IN;
        return 1;
 }
 
@@ -476,7 +476,6 @@ void process_chk(struct task *t, struct timeval *next)
                                                fdtab[fd].peeraddr = (struct sockaddr *)&sa;
                                                fdtab[fd].peerlen = sizeof(sa);
                                                fdtab[fd].state = FD_STCONN; /* connection in progress */
-                                               fdtab[fd].ev = 0;
                                                EV_FD_SET(fd, DIR_WR);  /* for connect status */
 #ifdef DEBUG_FULL
                                                assert (!EV_FD_ISSET(fd, DIR_RD));

diff --git a/src/client.c b/src/client.c
index d1d763db59a6f629c4c663fb73ec24e902feb2c9..bff5cd9ee9f91868000c413e565e5c8ac01e9d27 100644 (file)
--- a/src/client.c
+++ b/src/client.c
@@ -366,7 +366,6 @@ int event_accept(int fd) {
                fdtab[cfd].cb[DIR_WR].b = s->rep;
                fdtab[cfd].peeraddr = (struct sockaddr *)&s->cli_addr;
                fdtab[cfd].peerlen = sizeof(s->cli_addr);
-               fdtab[cfd].ev = 0;
 
                if ((p->mode == PR_MODE_HTTP && (s->flags & SN_MONITOR)) ||
                    (p->mode == PR_MODE_HEALTH && (p->options & PR_O_HTTP_CHK))) {

diff --git a/src/ev_sepoll.c b/src/ev_sepoll.c
index c58bf5394969f1509739ec719a341a2bfdfe8be5..61f1c6e57be1681c70c2371f490a79c8834fc5de 100644 (file)
--- a/src/ev_sepoll.c
+++ b/src/ev_sepoll.c
@@ -16,6 +16,7 @@
 
 #include <common/compat.h>
 #include <common/config.h>
+#include <common/debug.h>
 #include <common/standard.h>
 #include <common/time.h>
 #include <common/tools.h>
@@ -285,7 +286,7 @@ REGPRM2 static void _do_poll(struct poller *p, struct timeval *exp)
                 * the WAIT status.
                 */
 
-               fdtab[fd].ev = 0;
+               fdtab[fd].ev &= FD_POLL_STICKY;
                if ((eo & FD_EV_MASK_R) == FD_EV_SPEC_R) {
                        /* The owner is interested in reading from this FD */
                        if (fdtab[fd].state != FD_STCLOSE && fdtab[fd].state != FD_STERROR) {
@@ -412,7 +413,12 @@ REGPRM2 static void _do_poll(struct poller *p, struct timeval *exp)
                /* it looks complicated but gcc can optimize it away when constants
                 * have same values.
                 */
-               fdtab[fd].ev = 
+               DPRINTF(stderr, "%s:%d: fd=%d, ev=0x%08x, e=0x%08x\n",
+                       __FUNCTION__, __LINE__,
+                       fd, fdtab[fd].ev, e);
+
+               fdtab[fd].ev &= FD_POLL_STICKY;
+               fdtab[fd].ev |= 
                        ((e & EPOLLIN ) ? FD_POLL_IN  : 0) |
                        ((e & EPOLLPRI) ? FD_POLL_PRI : 0) |
                        ((e & EPOLLOUT) ? FD_POLL_OUT : 0) |
@@ -422,14 +428,14 @@ REGPRM2 static void _do_poll(struct poller *p, struct timeval *exp)
                if ((fd_list[fd].e & FD_EV_MASK_R) == FD_EV_WAIT_R) {
                        if (fdtab[fd].state == FD_STCLOSE || fdtab[fd].state == FD_STERROR)
                                continue;
-                       if (fdtab[fd].ev & (FD_POLL_RD|FD_POLL_HUP|FD_POLL_ERR))
+                       if (fdtab[fd].ev & (FD_POLL_IN|FD_POLL_HUP|FD_POLL_ERR))
                                fdtab[fd].cb[DIR_RD].f(fd);
                }
 
                if ((fd_list[fd].e & FD_EV_MASK_W) == FD_EV_WAIT_W) {
                        if (fdtab[fd].state == FD_STCLOSE || fdtab[fd].state == FD_STERROR)
                                continue;
-                       if (fdtab[fd].ev & (FD_POLL_WR|FD_POLL_ERR))
+                       if (fdtab[fd].ev & (FD_POLL_OUT|FD_POLL_ERR))
                                fdtab[fd].cb[DIR_WR].f(fd);
                }
        }

diff --git a/src/proto_tcp.c b/src/proto_tcp.c
index 0891faa3bb3f7c42def84973525a6740b7edce1b..d122a03a135c7d295805566cf8a2b49800695986 100644 (file)
--- a/src/proto_tcp.c
+++ b/src/proto_tcp.c
@@ -271,7 +271,6 @@ int tcp_bind_listener(struct listener *listener, char *errmsg, int errlen)
        fdtab[fd].peeraddr = NULL;
        fdtab[fd].peerlen = 0;
        fdtab[fd].listener = NULL;
-       fdtab[fd].ev = 0;
  tcp_return:
        if (msg && errlen)
                strlcpy2(errmsg, msg, errlen);

diff --git a/src/proto_uxst.c b/src/proto_uxst.c
index d367e2bca9908704219e8a1ed533a49f7a9d184e..90547226889cc0c13dbea3d6c80f4a5d8fd3da70 100644 (file)
--- a/src/proto_uxst.c
+++ b/src/proto_uxst.c
@@ -276,7 +276,6 @@ static int uxst_bind_listener(struct listener *listener)
        fdtab[fd].peeraddr = NULL;
        fdtab[fd].peerlen = 0;
        fdtab[fd].listener = NULL;
-       fdtab[fd].ev = 0;
        return ERR_NONE;
 }
 
@@ -499,8 +498,6 @@ int uxst_event_accept(int fd) {
                fdtab[cfd].cb[DIR_WR].b = s->rep;
                fdtab[cfd].peeraddr = (struct sockaddr *)&s->cli_addr;
                fdtab[cfd].peerlen = sizeof(s->cli_addr);
-               fdtab[cfd].ev = 0;
-
 
                tv_eternity(&s->req->rex);
                tv_eternity(&s->req->wex);

diff --git a/src/stream_sock.c b/src/stream_sock.c
index 8c47d318cb48a42c16f116e32ec1b2f9de2f88f4..08cc65b65058c1262606301175e434e787c76cb2 100644 (file)
--- a/src/stream_sock.c
+++ b/src/stream_sock.c
@@ -21,6 +21,7 @@
 
 #include <common/compat.h>
 #include <common/config.h>
+#include <common/debug.h>
 #include <common/standard.h>
 #include <common/time.h>
 
@@ -41,25 +42,24 @@
  * otherwise.
  */
 int stream_sock_read(int fd) {
-       __label__ out_eternity, out_wakeup, out_error;
+       __label__ out_eternity, out_wakeup, out_shutdown_r, out_error;
        struct buffer *b = fdtab[fd].cb[DIR_RD].b;
        int ret, max, retval;
        int read_poll = MAX_READ_POLL_LOOPS;
 
 #ifdef DEBUG_FULL
-       fprintf(stderr,"stream_sock_read : fd=%d, owner=%p\n", fd, fdtab[fd].owner);
+       fprintf(stderr,"stream_sock_read : fd=%d, ev=0x%02x, owner=%p\n", fd, fdtab[fd].ev, fdtab[fd].owner);
 #endif
 
        retval = 1;
 
-       if (unlikely(fdtab[fd].ev & FD_POLL_HUP)) {
-               /* connection closed */
-               b->flags |= BF_READ_NULL;
-               goto out_eternity;
-       }
-       else if (unlikely(fdtab[fd].state == FD_STERROR || (fdtab[fd].ev & FD_POLL_ERR))) {
+       /* stop immediately on errors */
+       if (fdtab[fd].state == FD_STERROR || (fdtab[fd].ev & FD_POLL_ERR))
                goto out_error;
-       }
+
+       /* stop here if we reached the end of data */
+       if ((fdtab[fd].ev & (FD_POLL_IN|FD_POLL_HUP)) == FD_POLL_HUP)
+               goto out_shutdown_r;
 
        while (1) {
                /*
@@ -128,10 +128,15 @@ int stream_sock_read(int fd) {
 
                        /* if too many bytes were missing from last read, it means that
                         * it's pointless trying to read again because the system does
-                        * not have them in buffers.
+                        * not have them in buffers. BTW, if FD_POLL_HUP was present,
+                        * it means that we have reached the end and that the connection
+                        * is closed.
                         */
-                       if (ret < max)
+                       if (ret < max) {
+                               if (fdtab[fd].ev & FD_POLL_HUP)
+                                       goto out_shutdown_r;
                                break;
+                       }
 
                        /* generally if we read something smaller than 1 or 2 MSS,
                         * it means that it's not worth trying to read again. It may
@@ -147,8 +152,7 @@ int stream_sock_read(int fd) {
                }
                else if (ret == 0) {
                        /* connection closed */
-                       b->flags |= BF_READ_NULL;
-                       goto out_eternity;
+                       goto out_shutdown_r;
                }
                else if (errno == EAGAIN) {
                        /* Ignore EAGAIN but inform the poller that there is
@@ -180,14 +184,20 @@ int stream_sock_read(int fd) {
  out_wakeup:
        if (b->flags & BF_READ_STATUS)
                task_wakeup(fdtab[fd].owner);
-       fdtab[fd].ev &= ~FD_POLL_RD;
+       fdtab[fd].ev &= ~FD_POLL_IN;
        return retval;
 
+ out_shutdown_r:
+       fdtab[fd].ev &= ~FD_POLL_HUP;
+       b->flags |= BF_READ_NULL;
+       goto out_eternity;
+
  out_error:
        /* There was an error. we must wakeup the task. No need to clear
         * the events, the task will do it.
         */
        fdtab[fd].state = FD_STERROR;
+       fdtab[fd].ev &= ~FD_POLL_STICKY;
        b->flags |= BF_READ_ERROR;
        goto out_eternity;
 }
@@ -210,7 +220,7 @@ int stream_sock_write(int fd) {
 #endif
 
        retval = 1;
-       if (unlikely(fdtab[fd].state == FD_STERROR || (fdtab[fd].ev & FD_POLL_ERR)))
+       if (fdtab[fd].state == FD_STERROR || (fdtab[fd].ev & FD_POLL_ERR))
                goto out_error;
 
        while (1) {
@@ -337,7 +347,7 @@ int stream_sock_write(int fd) {
  out_wakeup:
        if (b->flags & BF_WRITE_STATUS)
                task_wakeup(fdtab[fd].owner);
-       fdtab[fd].ev &= ~FD_POLL_WR;
+       fdtab[fd].ev &= ~FD_POLL_OUT;
        return retval;
 
  out_error:
@@ -345,6 +355,7 @@ int stream_sock_write(int fd) {
         * the events, the task will do it.
         */
        fdtab[fd].state = FD_STERROR;
+       fdtab[fd].ev &= ~FD_POLL_STICKY;
        b->flags |= BF_WRITE_ERROR;
        goto out_eternity;