BUG/MEDIUM: peers: do not keep a dict entry cached when its update was not sent

When an update message carries a server_key which is not yet in the peer's
dictionary TX cache, peer_prepare_updatemsg() inserts it into the cache and
encodes the full value. But if the message does not fit in the applet's
output buffer, applet_putblk() returns -1 and the message is simply dropped,
to be built again on the next call since last_pushed was not advanced. At
this point the entry is found in the TX cache, so only its ID is emitted,
and the remote peer, which never received the value, resolves this ID to a
NULL (or stale) entry in its RX cache. The server_key of this entry is then
lost on the remote side, as well as for all subsequent updates referencing
the same server for the whole session's lifetime, or it may even point to
another server whose value previously occupied the same slot.

This is easily observed during a resync of a table storing server_key with a
few servers having long names: the first update carrying a new server name
that does not fit in the buffer is retried without its value. With a debug
print on the receiver, ~100 entries out of 400 were received with an unknown
dict ID in such a test.

Let's record the node newly inserted in the TX cache in the update params,
and remove it from the cache when the message could not be emitted, so that
the next attempt sends the full value again.

The issue exists since the dict cache was introduced in 2.0 with commit
8d78fa7de ("MINOR: peers: Make peers protocol support new "server_name" data
type."), so this should be backported to all supported versions.

diff --git a/src/peers.c b/src/peers.c
index d9de05fbb4cb160f15cdffe3cbd211598956d098..37dae2771a35afd23b88902dd4e63ec12ec9dce9 100644 (file)
--- a/src/peers.c
+++ b/src/peers.c
@@ -153,6 +153,7 @@ struct peer_prep_params {
                int use_identifier;
                int use_timed;
                struct peer *peer;
+               struct ebpt_node *new_de; /* dict entry newly cached, if any */
        } updt;
        struct {
                struct shared_table *shared_table;
@@ -265,6 +266,7 @@ static void peer_session_forceshutdown(struct peer *peer);
 
 static struct ebpt_node *dcache_tx_insert(struct dcache *dc,
                                           struct dcache_tx_entry *i);
+static inline void dcache_tx_cancel(struct dcache *dc, struct ebpt_node *o);
 static inline void flush_dcache(struct peer *peer);
 
 static const char *statuscode_str(int statuscode);
@@ -880,6 +882,8 @@ int peer_prepare_updatemsg(char *msg, size_t size, struct peer_prep_params *p)
                                                intencode(cde.id + 1, &cursor);
                                        }
                                        else {
+                                               /* new entry in the cache, to be removed if the message is not sent */
+                                               p->updt.new_de = cached_de;
                                                /* Leave enough room to encode the remaining data length. */
                                                end = beg = cursor + PEER_MSG_ENC_LENGTH_MAXLEN;
                                                /* Encode the dictionary entry key */
@@ -1384,9 +1388,19 @@ static inline int peer_send_updatemsg(struct shared_table *st, struct appctx *ap
                        .peer = appctx->svcctx,
                },
        };
+       int ret;
 
        TRACE_PROTO("send update message", PEERS_EV_SESS_IO|PEERS_EV_TX_MSG|PEERS_EV_PROTO_UPDATE, appctx, NULL, st);
-       return peer_send_msg(appctx, peer_prepare_updatemsg, &p);
+       ret = peer_send_msg(appctx, peer_prepare_updatemsg, &p);
+
+       /* If the message was not emitted, a dictionary entry which was newly
+        * inserted in the TX cache while building it must be removed from
+        * there, otherwise the next attempt would only send its ID and the
+        * peer would never learn its value.
+        */
+       if (ret <= 0 && p.updt.new_de)
+               dcache_tx_cancel(p.updt.peer->dcache, p.updt.new_de);
+       return ret;
 }
 
 /*
@@ -4090,6 +4104,19 @@ static struct ebpt_node *dcache_tx_insert(struct dcache *dc, struct dcache_tx_en
        return o;
 }
 
+/*
+ * Remove from <dc> cache part used upon transmission the <o> entry which was
+ * just returned by dcache_tx_insert() as a new entry, because the message
+ * carrying its value could not be sent. The slot is simply left empty.
+ */
+static inline void dcache_tx_cancel(struct dcache *dc, struct ebpt_node *o)
+{
+       ebpt_delete(o);
+       o->key = NULL;
+       if (dc->tx->prev_lookup == o)
+               dc->tx->prev_lookup = NULL;
+}
+
 /*
  * Allocate a dictionary cache for each peer of <peers> section.
  * Return 1 if succeeded, 0 if not.