BUG/MEDIUM: connection: ensure to always report the end of handshakes
Despite the previous commit working fine on all tests, it's still not
sufficient to completely address the problem. If the connection handler
is called with an event validating an L4 connection but some handshakes
remain (eg: accept-proxy), it will still wake the function up, which
will not report the activity, and will not detect a change once the
handshake it complete so it will not notify the ->wake() handler.
In fact the only reason why the ->wake() handler is still called here
is because after dropping the last handshake, we try to call ->recv()
and ->send() in turn and change the flags in order to detect a data
activity. But if for any reason the data layer is not interested in
reading nor writing, it will not get these events.
A cleaner way to address this is to call the ->wake() handler only
on definitive status changes (shut, error), on real data activity,
and on a complete connection setup, measured as CONNECTED with no
more handshake pending.
It could be argued that the handshake flags have to be made part of
the condition to set CO_FL_CONNECTED but that would currently break
a part of the health checks. Also a handshake could appear at any
moment even after a connection is established so we'd lose the
ability to detect a second end of handshake.
For now the situation around CO_FL_CONNECTED is not clean :
- session_accept() only sets CO_FL_CONNECTED if there's no pending
handshake ;
- conn_fd_handler() will set it once L4 and L6 are complete, which
will do what session_accept() above refrained from doing even if
an accept_proxy handshake is still pending ;
- ssl_sock_infocbk() and ssl_sock_handshake() consider that a
handshake performed with CO_FL_CONNECTED set is a renegociation ;
=> they should instead filter on CO_FL_WAIT_L6_CONN
- all ssl_fc_* sample fetch functions wait for CO_FL_CONNECTED before
accepting to fetch information
=> they should also get rid of any pending handshake
- smp_fetch_fc_rcvd_proxy() uses !CO_FL_CONNECTED instead of
CO_FL_ACCEPT_PROXY
- health checks (standard and tcp-checks) don't check for HANDSHAKE
and may report a successful check based on CO_FL_CONNECTED while
not yet done (eg: send buffer full on send_proxy).
This patch aims at solving some of these side effects in a backportable
way before this is reworked in depth :
- we need to call ->wake() to report connection success, measure
connection time, notify that the data layer is ready and update
the data layer after activity ; this has to be done either if
we switch from pending {L4,L6}_CONN to nothing with no handshakes
left, or if we notice some handshakes were pending and are now
done.
- we document that CO_FL_CONNECTED exactly means "L4 connection
setup confirmed at least once, L6 connection setup confirmed
at least once or not necessary, all this regardless of any
possibly remaining handshakes or future L6 negociations".
This patch also renames CO_FL_CONN_STATUS to the more explicit
CO_FL_NOTIFY_DATA, and works around the previous flags trick consiting
in setting an impossible combination of flags to notify the data layer,
by simply clearing the current flags.
This fix should be backported to 1.7, 1.6 and 1.5.
diff --git a/src/connection.c b/src/connection.c
index bc34ae7..ba9b675 100644
--- a/src/connection.c
+++ b/src/connection.c
@@ -100,19 +100,21 @@
*/
if (conn->xprt && fd_recv_ready(fd) &&
((conn->flags & (CO_FL_DATA_RD_ENA|CO_FL_WAIT_ROOM|CO_FL_ERROR|CO_FL_HANDSHAKE)) == CO_FL_DATA_RD_ENA)) {
- /* force detection of a flag change : it's impossible to have both
- * CONNECTED and WAIT_CONN so we're certain to trigger a change.
+ /* force reporting of activity by clearing the previous flags :
+ * we'll have at least ERROR or CONNECTED at the end of an I/O,
+ * both of which will be detected below.
*/
- flags = CO_FL_WAIT_L4_CONN | CO_FL_CONNECTED;
+ flags = 0;
conn->data->recv(conn);
}
if (conn->xprt && fd_send_ready(fd) &&
((conn->flags & (CO_FL_DATA_WR_ENA|CO_FL_WAIT_DATA|CO_FL_ERROR|CO_FL_HANDSHAKE)) == CO_FL_DATA_WR_ENA)) {
- /* force detection of a flag change : it's impossible to have both
- * CONNECTED and WAIT_CONN so we're certain to trigger a change.
+ /* force reporting of activity by clearing the previous flags :
+ * we'll have at least ERROR or CONNECTED at the end of an I/O,
+ * both of which will be detected below.
*/
- flags = CO_FL_WAIT_L4_CONN | CO_FL_CONNECTED;
+ flags = 0;
conn->data->send(conn);
}
@@ -130,21 +132,30 @@
if (!tcp_connect_probe(conn))
goto leave;
}
-
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.
- */
+ /* 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;
- /* 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.
+ /* The wake callback is normally used to notify the data layer about
+ * data layer activity (successful send/recv), connection establishment,
+ * shutdown and fatal errors. We need to consider the following
+ * situations to wake up the data layer :
+ * - change among the CO_FL_NOTIFY_DATA flags :
+ * {DATA,SOCK}_{RD,WR}_SH, ERROR,
+ * - absence of any of {L4,L6}_CONN and CONNECTED, indicating the
+ * end of handshake and transition to CONNECTED
+ * - raise of CONNECTED with HANDSHAKE down
+ * - end of HANDSHAKE with CONNECTED set
+ * - regular data layer activity
+ *
+ * Note that the wake callback is allowed to release the connection and
+ * the fd (and return < 0 in this case).
*/
if ((conn->flags & CO_FL_WAKE_DATA) &&
- ((conn->flags ^ flags) & CO_FL_CONN_STATE) &&
+ (((conn->flags ^ flags) & CO_FL_NOTIFY_DATA) ||
+ ((flags & (CO_FL_CONNECTED|CO_FL_HANDSHAKE)) != CO_FL_CONNECTED &&
+ (conn->flags & (CO_FL_CONNECTED|CO_FL_HANDSHAKE)) == CO_FL_CONNECTED)) &&
conn->data->wake(conn) < 0)
return;