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/include/types/connection.h b/include/types/connection.h
index c644dd55bb5e1fd48ee5c7699869bfdcb299624b..657c9a1479da76d485e3f2e9dd01a9294b841e73 100644 (file)
--- a/include/types/connection.h
+++ b/include/types/connection.h
@@ -96,15 +96,15 @@ enum {
        CO_FL_SOCK_RD_SH    = 0x00040000,  /* SOCK layer was notified about shutr/read0 */
        CO_FL_SOCK_WR_SH    = 0x00080000,  /* SOCK layer asked for shutw */
 
-       /* flags used to report connection status and errors */
+       /* flags used to report connection errors or other closing conditions */
        CO_FL_ERROR         = 0x00100000,  /* a fatal error was reported     */
-       CO_FL_CONNECTED     = 0x00200000,  /* the connection is now established */
+       CO_FL_NOTIFY_DATA   = 0x001F0000,  /* any shut/error flags above needs to be reported */
+
+       /* flags used to report connection status updates */
+       CO_FL_CONNECTED     = 0x00200000,  /* L4+L6 now ready ; extra handshakes may or may not exist */
        CO_FL_WAIT_L4_CONN  = 0x00400000,  /* waiting for L4 to be connected */
        CO_FL_WAIT_L6_CONN  = 0x00800000,  /* waiting for L6 to be connected (eg: SSL) */
 
-       /* synthesis of the flags above */
-       CO_FL_CONN_STATE    = 0x00FF0000,  /* all shut/connected flags */
-
        /*** All the flags below are used for connection handshakes. Any new
         * handshake should be added after this point, and CO_FL_HANDSHAKE
         * should be updated.

diff --git a/src/connection.c b/src/connection.c
index bc34ae76b0453851f42b3178923ac2cbf5481407..ba9b67545b775637371360dcb46f6d03a0fdc48a 100644 (file)
--- a/src/connection.c
+++ b/src/connection.c
@@ -100,19 +100,21 @@ void conn_fd_handler(int fd)
         */
        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 @@ void conn_fd_handler(int fd)
                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;