MINOR: stconn: prefer to replace endpoints rather than detaching them

sc_detach_endp() is used to detach an endpoint, after which we either
assign a new one or completely release the object. Thus, it is guaranteed
that an stconn may not be found with no endpoint. However this is true in
the regular code flow, not at the signal level. And running "show threads"
during a retry, or triggering watchdog warnings on it can witness such a
stream executing instructions at the end of sc_detach_endp() before the
new endp is assigned, causing segfaults during dumps as described in
github issue #3490.

The new sedesc is allocated before releasing the old one, so we have
everything to perform a replacement. For this reason, sc_detach_endp()
was now replaced with sc_replace_endp(). It takes an optional new
endpoint to assign. When detaching, we just continue to pass NULL.
But when switching, we pass the newly allocated one and an external
observer may never see a NULL in the variable.

Note that we don't use atomics here (not really needed, and not enabled
without threads anyway). No failure was observed with millions of thread
dumps and millions of dumped streams. But if it were to still happen on
some specific archs, it would be fine to switch to an atomic store (it
was tested and works as well).

This may have to be backported to a few stable versions if it helps
improve the overall reliability during dumps, but let's wait a bit
first.

diff --git a/src/stconn.c b/src/stconn.c
index 8dcdd49700acce62aa99088b3c7cfda43cd03635..3aa5be486639cf8f36478933b9eef92ba77410ab 100644 (file)
--- a/src/stconn.c
+++ b/src/stconn.c
@@ -388,14 +388,15 @@ int sc_attach_hstream(struct stconn *sc, struct hstream *hs)
        return 0;
 }
 
-/* Detaches the stconn from the endpoint, if any. For a connecrion, if a
+/* Detaches the stconn from the endpoint, if any, and replace it with
+ * <newse> (which must be NULL for a pure detach). For a connection, if a
  * mux owns the connection ->detach() callback is called. Otherwise, it means
  * the stream connector owns the connection. In this case the connection is closed
  * and released. For an applet, the appctx is released. If still allocated, the
  * endpoint is reset and flag as detached. If the app layer is also detached,
  * the stream connector is released.
  */
-static void sc_detach_endp(struct stconn **scp)
+static void sc_replace_endp(struct stconn **scp, struct sedesc *newse)
 {
        struct stconn *sc = *scp;
        struct xref *peer;
@@ -418,7 +419,7 @@ static void sc_detach_endp(struct stconn **scp)
                                conn->mux->unsubscribe(sc, sc->wait_event.events, &sc->wait_event);
                        se_fl_set(sedesc, SE_FL_ORPHAN);
                        sedesc->sc = NULL;
-                       sc->sedesc = NULL;
+                       sc->sedesc = newse;
                        CALL_MUX_NO_RET(conn->mux, detach(sedesc));
                }
                else {
@@ -437,12 +438,12 @@ static void sc_detach_endp(struct stconn **scp)
 
                sc_ep_set(sc, SE_FL_ORPHAN);
                sc->sedesc->sc = NULL;
-               sc->sedesc = NULL;
+               sc->sedesc = newse;
                se_shutdown(appctx->sedesc, SE_SHR_RESET|SE_SHW_NORMAL);
                appctx_free(appctx);
        }
 
-       if (sc->sedesc) {
+       if (sc->sedesc != newse) {
                /* the SD wasn't used and can be recycled */
                sc->sedesc->se     = NULL;
                sc->sedesc->conn   = NULL;
@@ -482,7 +483,7 @@ static void sc_detach_app(struct stconn **scp)
  */
 void sc_destroy(struct stconn *sc)
 {
-       sc_detach_endp(&sc);
+       sc_replace_endp(&sc, NULL);
        sc_detach_app(&sc);
        BUG_ON_HOT(sc);
 }
@@ -503,7 +504,7 @@ int sc_reset_endp(struct stconn *sc)
                 * reset. The app is still attached, the sc will not be
                 * released.
                 */
-               sc_detach_endp(&sc);
+               sc_replace_endp(&sc, NULL);
                return 0;
        }
 
@@ -514,10 +515,9 @@ int sc_reset_endp(struct stconn *sc)
                return -1;
 
        /* The app is still attached, the sc will not be released */
-       sc_detach_endp(&sc);
+       sc_replace_endp(&sc, new_sd);
        BUG_ON(!sc);
-       BUG_ON(sc->sedesc);
-       sc->sedesc = new_sd;
+       BUG_ON(sc->sedesc != new_sd);
        sc->sedesc->sc = sc;
        sc->bytes_in = sc->bytes_out = 0;
        sc_ep_set(sc, SE_FL_DETACHED);
@@ -527,7 +527,7 @@ int sc_reset_endp(struct stconn *sc)
 
 /* Create an applet to handle a stream connector as a new appctx. The SC will
  * wake it up every time it is solicited. The appctx must be deleted by the task
- * handler using sc_detach_endp(), possibly from within the function itself.
+ * handler using sc_replace_endp(), possibly from within the function itself.
  * It also pre-initializes the applet's context and returns it (or NULL in case
  * it could not be allocated).
  */
@@ -2238,7 +2238,7 @@ void sc_conn_commit_endp_upgrade(struct stconn *sc)
 {
        if (!sc_ep_test(sc, SE_FL_DETACHED))
                return;
-       sc_detach_endp(&sc);
+       sc_replace_endp(&sc, NULL);
        /* Because it was already set as detached, the sedesc must be preserved */
        BUG_ON(!sc);
        BUG_ON(!sc->sedesc);