From: Aurelien DARRAGON <adarragon@haproxy.com>
Date: Wed, 24 Jul 2024 13:57:04 +0000 (+0200)
Subject: MEDIUM: sink: assume sft appctx stickiness
X-Git-Tag: v3.1-dev5~83
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=e328056ddc5029af0b34b3c8d757c49b33b7be76;p=thirdparty%2Fhaproxy.git

MEDIUM: sink: assume sft appctx stickiness

As mentioned in b40d804 ("MINOR: sink: add some comments about sft->appctx
usage in applet handlers"), there are few places in the code where it
looks like we assumed that the applet callbacks such as
sink_forward_session_init() or sink_forward_io_handler() could be
executing an appctx whose sft is detached from the appctx
(appctx != sft->appctx).

In practise this should not be happening since an appctx sticks to the
same thread its entire lifetime, and the only times sft->appctx is
effectively assigned is during the session/appctx creation (in
process_sink_forward()) or release.

Thus if sft->appctx wouldn't point to the appctx that the sft was bound
to after appctx creation, it would probably indicate a bug rather than
an expected condition. To further emphasize that and prevent the
confusion, and since 3.1-dev4 was released, let's remove such checks and
instead add a BUG_ON to ensure this never happens.

In _sink_forward_io_handler(), the "hard_close" label was removed since
there are no more uses for it (no hard errors may be caught from the
function for now)
---

diff --git a/src/sink.c b/src/sink.c
index 1b09c04aeb..db98fcebc8 100644
--- a/src/sink.c
+++ b/src/sink.c
@@ -375,11 +375,7 @@ static void _sink_forward_io_handler(struct appctx *appctx,
 	}
 
 	HA_SPIN_LOCK(SFT_LOCK, &sft->lock);
-	if (appctx != sft->appctx) {
-		/* FIXME: is this even supposed to happen? */
-		HA_SPIN_UNLOCK(SFT_LOCK, &sft->lock);
-		goto hard_close;
-	}
+	BUG_ON(appctx != sft->appctx);
 
 	MT_LIST_DELETE(&appctx->wait_entry);
 
@@ -426,10 +422,15 @@ out:
 	return;
 
 soft_close:
+	/* be careful: since the socket lacks the NOLINGER flag (on purpose)
+	 * soft_close will result in the port staying in TIME_WAIT state:
+	 * don't abuse from soft_close!
+	 */
 	se_fl_set(appctx->sedesc, SE_FL_EOS|SE_FL_EOI);
-	return;
-hard_close:
-	se_fl_set(appctx->sedesc, SE_FL_EOS|SE_FL_ERROR);
+	/* if required, hard_close could be achieve by using SE_FL_EOS|SE_FL_ERROR
+	 * flag combination: RST will be sent, TIME_WAIT will be avoided as if
+	 * we performed a normal close with NOLINGER flag set
+	 */
 }
 
 /*
@@ -476,6 +477,8 @@ static int sink_forward_session_init(struct appctx *appctx)
 	 */
 	HA_SPIN_LOCK(SFT_LOCK, &sft->lock);
 
+	BUG_ON(sft->appctx != appctx);
+
 	if (!sockaddr_alloc(&addr, &sft->srv->addr, sizeof(sft->srv->addr)))
 		goto out_error;
 	/* srv port should be learned from srv->svc_port not from srv->addr */
@@ -496,9 +499,6 @@ static int sink_forward_session_init(struct appctx *appctx)
 
 	applet_expect_no_data(appctx);
 
-	/* FIXME: redundant? was already assigned in process_sink_forward() */
-	sft->appctx = appctx;
-
 	HA_SPIN_UNLOCK(SFT_LOCK, &sft->lock);
 
 	return 0;
@@ -518,9 +518,8 @@ static void sink_forward_session_release(struct appctx *appctx)
 		return;
 
 	HA_SPIN_LOCK(SFT_LOCK, &sft->lock);
-	if (sft->appctx == appctx)
-		__sink_forward_session_deinit(sft);
-	/* FIXME: is 'sft->appctx != appctx' even supposed to happen? */
+	BUG_ON(sft->appctx != appctx);
+	__sink_forward_session_deinit(sft);
 	HA_SPIN_UNLOCK(SFT_LOCK, &sft->lock);
 }
 
@@ -609,7 +608,11 @@ static struct task *process_sink_forward(struct task * task, void *context, unsi
 	if (!stopping) {
 		while (sft) {
 			HA_SPIN_LOCK(SFT_LOCK, &sft->lock);
-			/* if appctx is NULL, start a new session */
+			/* If appctx is NULL, start a new session and perform the appctx
+			 * assigment right away since the applet is not supposed to change
+			 * during the session lifetime. By doing the assignment now we
+			 * make sure to start the session exactly once.
+			 */
 			if (!sft->appctx)
 				sft->appctx = sink_forward_session_create(sink, sft);
 			HA_SPIN_UNLOCK(SFT_LOCK, &sft->lock);