Fix a regression in #223: sanitise selected closures

[vnxme]

I've tracked the regression in ff5d983 to at least 2 rows of the code ff5d983#diff-ba204f43657cfc2b9fd32beb6baad78efc8ca53ff8dfe9ad4eb748f8f9395e36R79 and ff5d983#diff-ba204f43657cfc2b9fd32beb6baad78efc8ca53ff8dfe9ad4eb748f8f9395e36R83.

This PR sanitises all the closures added in #223 and resolves the hanging connections bug.

I believe it will also resolve the problem reported by @lxhao61 in ff5d983#comments.

[mholt]

Interesting, that fixes the bugs? I am surprised since I don't think those variables actually change outside the goroutine, so the value copied into the goroutines shouldn't be different than what is in the outer scope.

[lxhao61]

I've tracked the regression in ff5d983 to at least 2 rows of the code ff5d983#diff-ba204f43657cfc2b9fd32beb6baad78efc8ca53ff8dfe9ad4eb748f8f9395e36R79 and ff5d983#diff-ba204f43657cfc2b9fd32beb6baad78efc8ca53ff8dfe9ad4eb748f8f9395e36R83.

This PR sanitises all the closures added in #223 and resolves the hanging connections bug.

I believe it will also resolve the problem reported by @lxhao61 in ff5d983#comments.

This update does not fully fix it, only the SNI configuration is fixed, the UDP forwarding configuration is not fixed.

[vnxme]

@lxhao61 Let's continue our discussion here. Could you please explain how you're testing and what's exactly not working?

I'm trying to simulate your config, and the Caddyfile below works fine for me with this PR, i.e. it's correctly responding to https://abc.caddy, https://def.caddy, and DNS requests sent over UDP to 127.0.0.1:443.

{
	layer4 {
		:443 {
			@abc tls sni abc.caddy
			route @abc {
				proxy localhost:5443
			}
			@def tls sni def.caddy
			route @def {
				proxy localhost:6443
			}
		}
		udp/:443 {
			route {
				proxy udp/1.1.1.1:53
			}
		}
	}
}

abc.caddy:5443 {
	tls {
		issuer internal
	}
	respond "5443" 200
}

def.caddy:6443 {
	tls {
		issuer internal
	}
	respond "6443" 200
}

[vnxme]

@lxhao61 I've copied your Caddyfile config and it seems to work fine for me. Both abc.caddy and def.caddy are accessible at port 443. I'm not sure how to test h3 on Windows localhost, but I doubt ff5d983 could have caused any regression with regards to quic support that hasn't been addressed by this PR.

Please make sure you are testing Caddy with this PR and explain precisely what used to work and isn't working now for you.

[lxhao61]

@vnxme I know that the SNI configuration is normal now (both abc.caddy and def.caddy can be accessed through TCP 443), but the UDP 443 forwarding configuration is still abnormal, that is, def.caddy still does not support HTTP/3.
I don't know how to test HTTP/3 locally, I only know to test HTTP/3 on the Internet. The example that supports Internet testing is as follows (modified):

{
	email [email protected]

	layer4 {
		:443 {
			@abc tls sni abc.caddy
			route @abc {
				proxy {
					upstream 127.0.0.1:5443
					proxy_protocol v1
				}
			}
			route {
				proxy {
					upstream 127.0.0.1:7443
					proxy_protocol v1
				}
			}
		}
		udp/:443 {
			route {
				proxy udp/127.0.0.1:7443
			}
		}
	}

	servers 127.0.0.1:5443 {
		listener_wrappers {
			proxy_protocol
			tls
		}
		protocols h1 h2
	}

	servers 127.0.0.1:7443 {
		listener_wrappers {
			proxy_protocol
			tls
		}
		protocols h1 h2 h3
	}
}

abc.caddy:5443 {
	bind 127.0.0.1
	file_server {
		root /var/www/html
	}
}

def.caddy:7443 {
	bind 127.0.0.1
	header Alt-Svc "h3=\":443\"; ma=2592000"
	file_server {
		root /var/www/html
	}
}

I am absolutely sure that this is a bug introduced by ff5d983, and everything works fine after rolling back to b79350a. You can test it yourself.

[vnxme]

@lxhao61 OK, I've also tested http/3, and it also works with this PR. You didn't mention in your previous message that you actually tested this PR, only ff5d983 having a bug and b79350a. And I agree, rolling back to the latter will resolve your problem. But what about this PR?

I've tested your config with the following docker images in https://hub.docker.com/r/vnxme/caddy/tags:

• vnxme/caddy:sha-4100720 is Caddy v2.8.4 + b79350a. You're correct, your config works, QUIC support is green.
• vnxme/caddy:sha-cf58554 is Caddy v2.8.4 + ff5d983. You're correct, your config won't work because of the race condition bug that causes hanging connections. No QUIC support.
• vnxme/caddy:sha-1b81e99 is Caddy v2.8.4 + this PR. Your config works, QUIC support is green.

In case you would like to build your own Caddy with this PR, do like this:

--with github.com/mholt/caddy-l4=github.com/vnxme/caddy-l4@c4d1d6eea724d31bca0a7dd1671da1e75032faf2

@mholt, do you have any objections to merging this PR?

[mholt]

@mholt, do you have any objections to merging this PR?

I do not.

I'm still a little confused as to how it actually fixes a problem 😅 since I'm not seeing where those variables would change from the time of the outer evaluation and the inner, but we can continue that discussion on Slack too. I was away this weekend and am just catching up :)

[mholt]

OH, nevermind, I'm an idiot -- there are two loops! I see now that s is a loop var. Makes total sense now 👍