From: Willy Tarreau Date: Fri, 4 Sep 2026 10:07:55 +0000 (+0200) Subject: MINOR: stconn: prefer to replace endpoints rather than detaching them X-Git-Tag: v3.5-dev7~70 X-Git-Url: https://git.kaiwu.me/$%7BGITURL%7D$%7Bcid%7D?a=commitdiff_plain;h=75e93d71dd3ecb8411080b62df96b5479b0570ac;p=haproxy.git 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 8dcdd4970..3aa5be486 100644 --- 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 + * (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);