[MAJOR] buffers: fix misuse of the BF_SHUTW_NOW flag

This flag was incorrectly used as meaning "close immediately",
while it needs to say "close ASAP". ASAP here means when unsent
data pending in the buffer are sent. This helps cleaning up some
dirty tricks where the buffer output was checking the BF_SHUTR
flag combined with EMPTY and other such things. Now we have a
clearly defined semantics :

  - producer sets SHUTR and *may* set SHUTW_NOW if WRITE_ENA is
    set, otherwise leave it to the session processor to set it.
  - consumer only checks SHUTW_NOW to decide whether or not to
    call shutw().

This also induced very minor changes at some locations which were
not protected against buffer changes while the SHUTW_NOW flag was
set. Now we prevent send_max from changing when the flag is set.

Several tests have been run without any unexpected behaviour detected.

Some more cleanups are needed, as it clearly appears that some tests
could be removed with stricter semantics.

diff --git a/include/types/buffers.h b/include/types/buffers.h
index 84929485e352fc872bdc84313bf8db95f782b172..1d15c708779afdb9952e93afe887746efaa474ce 100644 (file)
--- a/include/types/buffers.h
+++ b/include/types/buffers.h
@@ -46,8 +46,9 @@
  *     BF_SHUT*_NOW, BF_*_ENA, BF_HIJACK
  *
  * The flags have been arranged for readability, so that the read and write
- * bits have se same position in a byte (read being the lower byte and write
- * the second one).
+ * bits have the same position in a byte (read being the lower byte and write
+ * the second one). All flag names are relative to the buffer. For instance,
+ * 'write' indicates the direction from the buffer to the stream interface.
  */
 
 #define BF_READ_NULL      0x000001  /* last read detected on producer side */
@@ -58,7 +59,7 @@
 
 #define BF_FULL           0x000010  /* buffer cannot accept any more data (l >= max_len) */
 #define BF_SHUTR          0x000020  /* producer has already shut down */
-#define BF_SHUTR_NOW      0x000040  /* the producer must shut down for reads immediately */
+#define BF_SHUTR_NOW      0x000040  /* the producer must shut down for reads ASAP */
 #define BF_READ_NOEXP     0x000080  /* producer should not expire */
 
 #define BF_WRITE_NULL     0x000100  /* write(0) or connect() succeeded on consumer side */
@@ -69,17 +70,40 @@
 
 #define BF_EMPTY          0x001000  /* buffer is empty */
 #define BF_SHUTW          0x002000  /* consumer has already shut down */
-#define BF_SHUTW_NOW      0x004000  /* the consumer must shut down for writes immediately */
+#define BF_SHUTW_NOW      0x004000  /* the consumer must shut down for writes ASAP */
 #define BF_WRITE_ENA      0x008000  /* consumer is allowed to forward all buffer contents */
 
+/* When either BF_SHUTR_NOW or BF_HIJACK is set, it is strictly forbidden for
+ * the producer to alter the buffer contents. When BF_SHUTW_NOW is set, the
+ * consumer is free to perform a shutw() when it has consumed the last contents,
+ * otherwise the session processor will do it anyway.
+ *
+ * The SHUT* flags work like this :
+ *
+ *  SHUTR SHUTR_NOW  meaning
+ *    0       0      normal case, connection still open and data is being read
+ *    0       1      closing : the producer cannot feed data anymore but can close
+ *    1       0      closed: the producer has closed its input channel.
+ *    1       1      impossible
+ *
+ *  SHUTW SHUTW_NOW  meaning
+ *    0       0      normal case, connection still open and data is being written
+ *    0       1      closing: the consumer can send last data and may then close
+ *    1       0      closed: the consumer has closed its output channel.
+ *    1       1      impossible
+ *
+ * The SHUTW_NOW flag should be set by the session processor when SHUTR and WRITE_ENA
+ * are both set. It may also be set by a hijacker at the end of data. And it may also
+ * be set by the producer when it detects SHUTR while directly forwarding data to the
+ * consumer.
+ *
+ * The SHUTR_NOW flag is mostly used to force the producer to abort when an error is
+ * detected on the consumer side.
+ */
+
 #define BF_STREAMER       0x010000  /* the producer is identified as streaming data */
 #define BF_STREAMER_FAST  0x020000  /* the consumer seems to eat the stream very fast */
 
-/* When either BF_SHUTR_NOW or BF_HIJACK is set, it is strictly forbidden for
- * the stream interface to alter the buffer contents. When BF_SHUTW_NOW is set,
- * it is strictly forbidden for the stream interface to send anything from the
- * buffer.
- */
 #define BF_HIJACK         0x040000  /* the producer is temporarily replaced by ->hijacker */
 #define BF_ANA_TIMEOUT    0x080000  /* the analyser timeout has expired */
 #define BF_READ_ATTACHED  0x100000  /* the read side is attached for the first time */

diff --git a/src/session.c b/src/session.c
index 3e7484dbea87290d5275400a33eb8a348b538434..013be7f029b1a8bb4ad6718d43684f1e28738661 100644 (file)
--- a/src/session.c
+++ b/src/session.c
@@ -198,9 +198,8 @@ int sess_update_st_con_tcp(struct session *s, struct stream_interface *si)
        }
 
        /* OK, maybe we want to abort */
-       if (unlikely((req->flags & BF_SHUTW_NOW) ||
-                    (rep->flags & BF_SHUTW) ||
-                    ((req->flags & BF_SHUTR) && /* FIXME: this should not prevent a connection from establishing */
+       if (unlikely((rep->flags & BF_SHUTW) ||
+                    ((req->flags & BF_SHUTW_NOW) && /* FIXME: this should not prevent a connection from establishing */
                      (((req->flags & (BF_EMPTY|BF_WRITE_ACTIVITY)) == BF_EMPTY) ||
                       s->be->options & PR_O_ABRT_CLOSE)))) {
                /* give up */
@@ -451,8 +450,8 @@ void sess_update_stream_int(struct session *s, struct stream_interface *si)
                }
 
                /* Connection remains in queue, check if we have to abort it */
-               if ((si->ob->flags & (BF_READ_ERROR|BF_SHUTW_NOW)) || /* abort requested */
-                   ((si->ob->flags & BF_SHUTR) &&           /* empty and client stopped */
+               if ((si->ob->flags & (BF_READ_ERROR)) ||
+                   ((si->ob->flags & BF_SHUTW_NOW) &&   /* empty and client aborted */
                     (si->ob->flags & BF_EMPTY || s->be->options & PR_O_ABRT_CLOSE))) {
                        /* give up */
                        si->exp = TICK_ETERNITY;
@@ -471,8 +470,8 @@ void sess_update_stream_int(struct session *s, struct stream_interface *si)
        }
        else if (si->state == SI_ST_TAR) {
                /* Connection request might be aborted */
-               if ((si->ob->flags & (BF_READ_ERROR|BF_SHUTW_NOW)) || /* abort requested */
-                   ((si->ob->flags & BF_SHUTR) &&           /* empty and client stopped */
+               if ((si->ob->flags & (BF_READ_ERROR)) ||
+                   ((si->ob->flags & BF_SHUTW_NOW) &&  /* empty and client aborted */
                     (si->ob->flags & BF_EMPTY || s->be->options & PR_O_ABRT_CLOSE))) {
                        /* give up */
                        si->exp = TICK_ETERNITY;
@@ -957,7 +956,7 @@ resync_stream_interface:
                 * of data to be forwarded from the producer to the consumer (which
                 * might possibly not be connected yet).
                 */
-               if (!(s->req->flags & BF_SHUTR) &&
+               if (!(s->req->flags & (BF_SHUTR|BF_SHUTW|BF_SHUTW_NOW)) &&
                    s->req->to_forward < FORWARD_DEFAULT_SIZE)
                        buffer_forward(s->req, FORWARD_DEFAULT_SIZE);
        }
@@ -982,10 +981,10 @@ resync_stream_interface:
         */
 
        /* first, let's check if the request buffer needs to shutdown(write) */
-       if (unlikely((s->req->flags & (BF_SHUTW|BF_SHUTW_NOW|BF_EMPTY|BF_HIJACK|BF_WRITE_ENA|BF_SHUTR)) ==
-                    (BF_EMPTY|BF_WRITE_ENA|BF_SHUTR)))
+       if (unlikely((s->req->flags & (BF_SHUTW|BF_SHUTW_NOW|BF_HIJACK|BF_WRITE_ENA|BF_SHUTR)) ==
+                    (BF_WRITE_ENA|BF_SHUTR)))
                buffer_shutw_now(s->req);
-       else if ((s->req->flags & (BF_SHUTW|BF_SHUTW_NOW|BF_EMPTY|BF_WRITE_ENA)) == (BF_EMPTY|BF_WRITE_ENA) &&
+       else if ((s->req->flags & (BF_SHUTW|BF_SHUTW_NOW|BF_WRITE_ENA)) == (BF_WRITE_ENA) &&
                 (s->req->cons->state == SI_ST_EST) &&
                 s->be->options & PR_O_FORCE_CLO &&
                 s->rep->flags & BF_READ_ACTIVITY) {
@@ -997,7 +996,7 @@ resync_stream_interface:
        }
 
        /* shutdown(write) pending */
-       if (unlikely((s->req->flags & (BF_SHUTW|BF_SHUTW_NOW)) == BF_SHUTW_NOW))
+       if (unlikely((s->req->flags & (BF_SHUTW|BF_SHUTW_NOW|BF_EMPTY)) == (BF_SHUTW_NOW|BF_EMPTY)))
                s->req->cons->shutw(s->req->cons);
 
        /* shutdown(write) done on server side, we must stop the client too */
@@ -1072,7 +1071,7 @@ resync_stream_interface:
                 * of data to be forwarded from the producer to the consumer (which
                 * might possibly not be connected yet).
                 */
-               if (!(s->rep->flags & BF_SHUTR) &&
+               if (!(s->rep->flags & (BF_SHUTR|BF_SHUTW|BF_SHUTW_NOW)) &&
                    s->rep->to_forward < FORWARD_DEFAULT_SIZE)
                        buffer_forward(s->rep, FORWARD_DEFAULT_SIZE);
        }
@@ -1101,12 +1100,12 @@ resync_stream_interface:
         */
 
        /* first, let's check if the response buffer needs to shutdown(write) */
-       if (unlikely((s->rep->flags & (BF_SHUTW|BF_SHUTW_NOW|BF_EMPTY|BF_HIJACK|BF_WRITE_ENA|BF_SHUTR)) ==
-                    (BF_EMPTY|BF_WRITE_ENA|BF_SHUTR)))
+       if (unlikely((s->rep->flags & (BF_SHUTW|BF_SHUTW_NOW|BF_HIJACK|BF_WRITE_ENA|BF_SHUTR)) ==
+                    (BF_WRITE_ENA|BF_SHUTR)))
                buffer_shutw_now(s->rep);
 
        /* shutdown(write) pending */
-       if (unlikely((s->rep->flags & (BF_SHUTW|BF_SHUTW_NOW)) == BF_SHUTW_NOW))
+       if (unlikely((s->rep->flags & (BF_SHUTW|BF_EMPTY|BF_SHUTW_NOW)) == (BF_EMPTY|BF_SHUTW_NOW)))
                s->rep->cons->shutw(s->rep->cons);
 
        /* shutdown(write) done on the client side, we must stop the server too */

diff --git a/src/stream_sock.c b/src/stream_sock.c
index b2bacdcfbdd91fb26d4aa9b8839ceb06f10268e8..715259b2727610f2a3a3b1c1051265e2949a14d3 100644 (file)
--- a/src/stream_sock.c
+++ b/src/stream_sock.c
@@ -332,7 +332,7 @@ int stream_sock_read(int fd) {
                        cur_read += ret;
 
                        /* if we're allowed to directly forward data, we must update send_max */
-                       if (b->to_forward > 0) {
+                       if (b->to_forward > 0 && !(b->flags & (BF_SHUTW|BF_SHUTW_NOW))) {
                                int fwd = MIN(b->to_forward, ret);
                                b->send_max   += fwd;
                                b->to_forward -= fwd;
@@ -502,6 +502,8 @@ int stream_sock_read(int fd) {
        /* we received a shutdown */
        fdtab[fd].ev &= ~FD_POLL_HUP;
        b->flags |= BF_READ_NULL;
+       if (b->flags & BF_WRITE_ENA)
+               buffer_shutw_now(b);
        stream_sock_shutr(si);
        goto out_wakeup;
 
@@ -599,7 +601,7 @@ static int stream_sock_write_loop(struct stream_interface *si, struct buffer *b)
                        unsigned int send_flag = MSG_DONTWAIT | MSG_NOSIGNAL;
 
                        if (MSG_MORE &&
-                           (((b->flags & (BF_SHUTW|BF_SHUTW_NOW|BF_HIJACK|BF_WRITE_ENA|BF_SHUTR)) == (BF_WRITE_ENA|BF_SHUTR) &&
+                           (((b->flags & (BF_SHUTW|BF_SHUTW_NOW|BF_HIJACK|BF_WRITE_ENA)) == (BF_WRITE_ENA|BF_SHUTW_NOW) &&
                              (max == b->l)) ||
                             (max != b->l && max != b->send_max))
                            && (fdtab[si->fd].flags & FD_FL_TCP)) {
@@ -739,8 +741,8 @@ int stream_sock_write(int fd)
                 * send_max limit was reached. Maybe we just wrote the last
                 * chunk and need to close.
                 */
-               if (((b->flags & (BF_SHUTW|BF_EMPTY|BF_HIJACK|BF_WRITE_ENA|BF_SHUTR)) ==
-                    (BF_EMPTY|BF_WRITE_ENA|BF_SHUTR)) &&
+               if (((b->flags & (BF_SHUTW|BF_HIJACK|BF_WRITE_ENA|BF_SHUTW_NOW)) ==
+                    (BF_WRITE_ENA|BF_SHUTW_NOW)) &&
                    (si->state == SI_ST_EST)) {
                        stream_sock_shutw(si);
                        goto out_wakeup;
@@ -813,6 +815,7 @@ int stream_sock_write(int fd)
  */
 void stream_sock_shutw(struct stream_interface *si)
 {
+       si->ob->flags &= ~BF_SHUTW_NOW;
        if (si->ob->flags & BF_SHUTW)
                return;
        si->ob->flags |= BF_SHUTW;
@@ -857,6 +860,7 @@ void stream_sock_shutw(struct stream_interface *si)
  */
 void stream_sock_shutr(struct stream_interface *si)
 {
+       si->ib->flags &= ~BF_SHUTR_NOW;
        if (si->ib->flags & BF_SHUTR)
                return;
        si->ib->flags |= BF_SHUTR;
@@ -1041,8 +1045,8 @@ void stream_sock_chk_snd(struct stream_interface *si)
                 * send_max limit was reached. Maybe we just wrote the last
                 * chunk and need to close.
                 */
-               if (((ob->flags & (BF_SHUTW|BF_EMPTY|BF_HIJACK|BF_WRITE_ENA|BF_SHUTR)) ==
-                    (BF_EMPTY|BF_WRITE_ENA|BF_SHUTR)) &&
+               if (((ob->flags & (BF_SHUTW|BF_HIJACK|BF_WRITE_ENA|BF_SHUTW_NOW)) ==
+                    (BF_WRITE_ENA|BF_SHUTW_NOW)) &&
                    (si->state == SI_ST_EST)) {
                        stream_sock_shutw(si);
                        goto out_wakeup;