[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 8492948..1d15c70 100644
--- 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 3e7484d..013be7f 100644
--- a/src/session.c
+++ b/src/session.c
@@ -198,9 +198,8 @@
}
/* 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 @@
}
/* 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 @@
}
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 @@
* 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 @@
*/
/* 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 @@
}
/* 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 @@
* 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 @@
*/
/* 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 b2bacdc..715259b 100644
--- a/src/stream_sock.c
+++ b/src/stream_sock.c
@@ -332,7 +332,7 @@
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 @@
/* 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 @@
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 @@
* 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 @@
*/
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_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 @@
* 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;