You can not select more than 25 topics
Topics must start with a letter or number, can include dashes ('-') and can be up to 35 characters long.
100 lines
4.6 KiB
100 lines
4.6 KiB
From afbf56b951967e8fa4d509e423fdcb11c27d40e2 Mon Sep 17 00:00:00 2001
|
|
From: Willy Tarreau <w@1wt.eu>
|
|
Date: Tue, 14 Mar 2017 20:19:29 +0100
|
|
Subject: [PATCH 7/7] BUG/MAJOR: connection: update CO_FL_CONNECTED before
|
|
calling the data layer
|
|
|
|
Matthias Fechner reported a regression in 1.7.3 brought by the backport
|
|
of commit 819efbf ("BUG/MEDIUM: tcp: don't poll for write when connect()
|
|
succeeds"), causing some connections to fail to establish once in a while.
|
|
While this commit itself was a fix for a bad sequencing of connection
|
|
events, it in fact unveiled a much deeper bug going back to the connection
|
|
rework era in v1.5-dev12 : 8f8c92f ("MAJOR: connection: add a new
|
|
CO_FL_CONNECTED flag").
|
|
|
|
It's worth noting that in a lab reproducing a similar environment as
|
|
Matthias' about only 1 every 19000 connections exhibit this behaviour,
|
|
making the issue not so easy to observe. A trick to make the problem
|
|
more observable consists in disabling non-blocking mode on the socket
|
|
before calling connect() and re-enabling it later, so that connect()
|
|
always succeeds. Then it becomes 100% reproducible.
|
|
|
|
The problem is that this CO_FL_CONNECTED flag is tested after deciding to
|
|
call the data layer (typically the stream interface but might be a health
|
|
check as well), and that the decision to call the data layer relies on a
|
|
change of one of the flags covered by the CO_FL_CONN_STATE set, which is
|
|
made of CO_FL_CONNECTED among others.
|
|
|
|
Before the fix above, this bug couldn't appear with TCP but it could
|
|
appear with Unix sockets. Indeed, connect() was always considered
|
|
blocking so the CO_FL_WAIT_L4_CONN connection flag was always set, and
|
|
polling for write events was always enabled. This used to guarantee that
|
|
the conn_fd_handler() could detect a change among the CO_FL_CONN_STATE
|
|
flags.
|
|
|
|
Now with the fix above, if a connect() immediately succeeds for non-ssl
|
|
connection with send-proxy enabled, and no data in the buffer (thus TCP
|
|
mode only), the CO_FL_WAIT_L4_CONN flag is not set, the lack of data in
|
|
the buffer doesn't enable polling flags for the data layer, the
|
|
CO_FL_CONNECTED flag is not set due to send-proxy still being pending,
|
|
and once send-proxy is done, its completion doesn't cause the data layer
|
|
to be woken up due to the fact that CO_FL_CONNECT is still not present
|
|
and that the CO_FL_SEND_PROXY flag is not watched in CO_FL_CONN_STATE.
|
|
|
|
Then no progress is made when data are received from the client (and
|
|
attempted to be forwarded), because a CF_WRITE_NULL (or CF_WRITE_PARTIAL)
|
|
flag is needed for the stream-interface state to turn from SI_ST_CON to
|
|
SI_ST_EST, allowing ->chk_snd() to be called when new data arrive. And
|
|
the only way to set this flag is to call the data layer of course.
|
|
|
|
After the connect timeout, the connection gets killed and if in the mean
|
|
time some data have accumulated in the buffer, the retry will succeed.
|
|
|
|
This patch fixes this situation by simply placing the update of
|
|
CO_FL_CONNECTED where it should have been, before the check for a flag
|
|
change needed to wake up the data layer and not after.
|
|
|
|
This fix must be backported to 1.7, 1.6 and 1.5. Versions not having
|
|
the patch above are still affected for unix sockets.
|
|
|
|
Special thanks to Matthias Fechner who provided a very detailed bug
|
|
report with a bisection designating the faulty patch, and to Olivier
|
|
Houchard for providing full access to a pretty similar environment where
|
|
the issue could first be reproduced.
|
|
(cherry picked from commit 7bf3fa3c23f6a1b7ed1212783507ac50f7e27544)
|
|
---
|
|
src/connection.c | 11 +++++++----
|
|
1 file changed, 7 insertions(+), 4 deletions(-)
|
|
|
|
diff --git a/src/connection.c b/src/connection.c
|
|
index 26fc5f6..1e4c9aa 100644
|
|
--- a/src/connection.c
|
|
+++ b/src/connection.c
|
|
@@ -131,6 +131,13 @@ void conn_fd_handler(int fd)
|
|
}
|
|
|
|
leave:
|
|
+ /* Verify if the connection just established. The CO_FL_CONNECTED flag
|
|
+ * being included in CO_FL_CONN_STATE, its change will be noticed by
|
|
+ * the next block and be used to wake up the data layer.
|
|
+ */
|
|
+ if (unlikely(!(conn->flags & (CO_FL_WAIT_L4_CONN | CO_FL_WAIT_L6_CONN | CO_FL_CONNECTED))))
|
|
+ conn->flags |= CO_FL_CONNECTED;
|
|
+
|
|
/* The wake callback may be used to process a critical error and abort the
|
|
* connection. If so, we don't want to go further as the connection will
|
|
* have been released and the FD destroyed.
|
|
@@ -140,10 +147,6 @@ void conn_fd_handler(int fd)
|
|
conn->data->wake(conn) < 0)
|
|
return;
|
|
|
|
- /* Last check, verify if the connection just established */
|
|
- if (unlikely(!(conn->flags & (CO_FL_WAIT_L4_CONN | CO_FL_WAIT_L6_CONN | CO_FL_CONNECTED))))
|
|
- conn->flags |= CO_FL_CONNECTED;
|
|
-
|
|
/* remove the events before leaving */
|
|
fdtab[fd].ev &= FD_POLL_STICKY;
|
|
|
|
--
|
|
2.10.2
|
|
|