MINOR: stream: decrement srv->served after detaching from the list

In commit 3372a2ea00 ("BUG/MEDIUM: queues: Stricly respect maxconn for
outgoing connections"), it has been ensured that srv->served is held
as long as possible around the periods where a stream is attached to a
server. However, it's decremented early when entering sess_change_server,
and actually just before detaching from that server's list. While there
is theoretically nothing wrong with this, it prevents us from looking at
this counter to know if streams are still using a server or not.

We could imagine decrementing it much later but that wouldn't work with
leastconn, since that algo needs ->served to be final before calling
lbprm.server_drop_conn(). Thus what we're doing here is to detach from
the server, then decrement ->served, and only then call the LB callback
to update the server's position in the tree. At this moment the stream
doesn't know the server anymore anyway (except via this function's
local variable) so it's safe to consider that no stream knows the server
once the variable reaches zero.

diff --git a/src/stream.c b/src/stream.c
index 2f3a381af2e90747b4bfa5915990496dd359ffe5..78a5b7e638dfc05a91291d882d62b80a62d1cdae 100644 (file)
--- a/src/stream.c
+++ b/src/stream.c
@@ -2749,22 +2749,32 @@ void sess_change_server(struct stream *strm, struct server *newsrv)
         * invocation of sess_change_server().
         */
 
-       /*
-        * It is assumed if the stream has a non-NULL srv_conn, then its
-        * served field has been incremented, so we have to decrement it now.
-        */
-       if (oldsrv)
-               _HA_ATOMIC_DEC(&oldsrv->served);
-
-       if (oldsrv == newsrv)
+       if (oldsrv == newsrv) {
+               /*
+                * It is assumed if the stream has a non-NULL srv_conn, then its
+                * served field has been incremented, so we have to decrement it now.
+                */
+               if (oldsrv)
+                       _HA_ATOMIC_DEC(&oldsrv->served);
                return;
+       }
 
        if (oldsrv) {
+               /* Note: we cannot decrement served after calling server_drop_conn
+                * because that one may rely on served (e.g. for leastconn). The
+                * real need here is to make sure that when served==0, no stream
+                * knows the server anymore, mainly for server removal purposes,
+                * and that removal will be done under isolation anyway. Thus by
+                * decrementing served after detaching from the list, we're
+                * guaranteeing that served==0 implies that no stream is in the
+                * list anymore, which is a sufficient guarantee for tha goal.
+                */
                _HA_ATOMIC_DEC(&oldsrv->proxy->served);
+               stream_del_srv_conn(strm);
+               _HA_ATOMIC_DEC(&oldsrv->served);
                __ha_barrier_atomic_store();
                if (oldsrv->proxy->lbprm.server_drop_conn)
                        oldsrv->proxy->lbprm.server_drop_conn(oldsrv);
-               stream_del_srv_conn(strm);
        }
 
        if (newsrv) {