caddyhttp: Fix HTTP/3 breaking after reload

[vladbondarenko]

Fixes #4348

Disabling extra closing http3 listeners.

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[mholt]

Ah, interesting. I don't seem to remember that being the case before, so that's good news. (Or if it was, I just made a dumb. But I swear this used to work, so. Anyway I'm not complaining. The best fixes delete code.)

Thanks for the PR! Will review this shortly.

[francislavoie]

@marten-seemann does this make sense to you?

[marten-seemann]

I feel like I'm missing context here. What do you mean by "as they already closing in lucas-clemente/[email protected]/http3/server.go L:413"? That would be Server.Close, but someone has to call that function, right?

[francislavoie]

@marten-seemann the issue this is meant to solve is #4348

I'll update the PR description/title.

[marten-seemann]

How does this reload work? Do you restart the QUIC server after the new config has been applied?

[francislavoie]

@marten-seemann during a reload, the old server is kept up, and a new one is created to replace it, and finally the old server will be torn down if the new one started without error. So two servers will be running concurrently for a moment. There's a more thorough explanation of Caddy's architecture here, to bring you back up to speed on this https://caddyserver.com/docs/architecture

[marten-seemann]

I'm not sure. Running them concurrently is ok, but you need to close the old server somehow, don't you?

[awfulcooking]

Here's a context summary, fwiw:

#2727 - HTTP3 implemented. There is trouble with reloading, relating to closing / recreating servers, listeners with quic-go

quic-go/quic-go#2111 - @marten-seemann attempts to fix an issue with quic-go, making its multiplexer recognise conns by their local address string, instead of struct equality.

#2727 comment from @mholt just before merge (Sept 2019) - "the issue persists, but by not closing the http3.Server, the issue is either resolved or it hides itself."

So it's merged, with h3 listeners being closed. A loop to close h3 servers is commented out.

18 Feb 2020, @mholt restores the loop that closes h3 servers. This diff, surrounding comments probably most salient for context.

#4348, "http/3 stops working after reload" filed 12 Sep 2021. Leading to this PR

[awfulcooking]

Could it be related to quic-go/quic-go#2111 preventing the new listener from starting?

[mholt]

@vladbondarenko Thanks for looking into this. When you do reloads, you should see some logs like:

[DEBUG] ... Usage counter should not go above 2 or maybe 3, is now: N

where N is a number that is hopefully less than 3. Does that number stay below 3 when you do a few reloads?

[emilylange]

18 Feb 2020, @mholt restores the loop that closes h3 servers. This diff, surrounding comments probably most salient for context.

The mentioned commit happened on tag v2.0.0-beta.15 and seems to go on par with what I wrote in #4348 (comment):

I went as far as v2.0.0 back (thus the two Caddyfiles, see further down) and encountered this issue every time

So I went and checked all the v2.0.0-beta.* but those seem to work just fine. The same applies to the actual v2.0.0 release.
But I could swear when I tried v2.0.0 (caddy:2.0.0 Docker Hub) before, it failed.

Thus, I redid my testing:

• Docker Hub
  • caddy:2.0.0 from Docker Hub seems to work
  • caddy:2.1.1 from Docker Hub seems to work
  • caddy:2.2.0 from Docker Hub does not to work
• GitHub Releases (Linux amd64/x86_64 builds)
  • v2.2.0-rc.1 h1:ULGBB9efRs8yG27IBOMLljhCa4iM1IluFTDxjm+Y8vE= seems to work
  • v2.2.0-rc.2 h1:TIZuB1xz2YEvoFOPSGuxjGAcS4lMP604V+rwL0X4V0Q= does not work
• go run ./main.go run
  • v2.2.0 build with go1.15.15 does not work
  • v2.2.0 built with go1.14.15 seems to work

Commit 0279a57 between v2.2.0-rc.1...v2.2.0-rc.2 updated quic-go and Go to 1.15

Could this be, perhaps, somewhat related to the Go version bump from 1.14 to 1.15?

[mholt]

Interesting. Thanks for the detective work. I am not sure what the upgrade from Go 1.14 to Go 1.15 would have done to affect that...

[mholt]

@vladbondarenko Can you confirm that the counter doesn't increase?

[ChrisLahaye]

Unfortunately this is still an issue for me. Is there anything needed before this can get merged?

[mholt]

We need confirmation that it does work and that the counter doesn't increase, indicating a leak.

[ChrisLahaye]

I will look into this and get back to you guys.

[ChrisLahaye]

I ran some tests and this PR indeed creates a leak.

It seems like that the h3servers (and h3listeners) need to be closed explicitly. Without that, the number of listeners keeps increasing. This has been validated by adding a probe to the ListenPacket function in listeners.go (see https://github.com/caddyserver/caddy/blob/master/listeners.go#L58):

+	pv, loaded := listenerPool.pool[lnKey]
+	if loaded {
+		log.Printf("[DEBUG] %s: loaded %d", lnKey, atomic.LoadInt32(&pv.refs))
+	}

I did observe that when using (*http.Server).Shutdown the issue does not happen. This function comes from the http.Server embed in http3.Server, so I am not sure how far this is compatible with the http3 resources. Therefore, I will refrain from a PR and just leave it here for discussion.

[mholt]

Thanks for verifying that @ChrisLahaye . Sounds like we'll need a way to shut down the HTTP/3 server without closing the socket for others who may be using it.

[mholt]

I'm going to close this PR for now as it seems like it may be going inactive, and it has been confirmed that it's not a viable fix. Please feel welcome to continue discussion though if needed, although perhaps that is more relevant in the original issue unless it's about this change specifically. Thanks!