Close() and CloseGracefully() feedback; use context.Context

[mholt]

Hi,

I look forward to CloseGracefully() being implemented sometime. Not urgent for now though. 😃

I also would suggest using context.Context for cancellation/timeout instead of specifying a duration. That's how net/http.Server.Shutdown works, and having API parity would be kind of nice. In my case, Caddy will be shutting down net/http.Servers in conjunction with http3.Servers, so doing it the same way by reusing the same context is appealing.

[mholt]

Also, I just noticed that when I call http3.Server.Close(), the underlying listener (net.PacketConn actually) that I passed into http3.Server.Serve() doesn't get closed. Well, maybe it does under the hood, but that listener's Close() method doesn't get called. This is problematic for me since Caddy does some bookkeeping as to when listeners get closed, etc, and if the listener I give the server never gets Closed, well... it leaks resources. (I wrap the standard lib net.PacketConn with my own type, which still satisfies the PacketConn interface.)

It seems like a call to the underlying listener's Close() should happen in here, regardless of whether ListenAddr was used? https://sourcegraph.com/github.com/lucas-clemente/quic-go@bcac555574071b6fa6af77468927194c4306b924/-/blob/server.go#L316-318

[mholt]

I did some further testing. That actual highlighted segment I linked to above is not the culprit, but a lack of s.conn.Close() does seem to be problematic. Adding s.conn.Close() right before or after the linked segment (but outside the if statement) successfully calls my Close() method.

So, is this something that can be added? Just the one line to close the underlying PacketConn?

[marten-seemann]

Also, I just noticed that when I call http3.Server.Close(), the underlying listener (net.PacketConn actually) that I passed into http3.Server.Serve() doesn't get closed. Well, maybe it does under the hood, but that listener's Close() method doesn't get called.

This is counterintuitive coming from TCP, but it makes sense for QUIC. With QUIC, you can (and quic-go supports this) run a server and multiple clients on the same underlying UDP "connection". This has a lot of advantages, especially for p2p use cases, and we already use that feature in libp2p.
The same applies to HTTP/3. You could run an HTTP server and HTTP clients on the same net.PacketConn.

That's why we have the check you highlighted in https://github.com/lucas-clemente/quic-go/blob/master/server.go#L314-L318
If the server created the net.PacketConn itself, we are certain that it's not used for any outgoing connections, so we can safely close it. If the user gave us the net.PacketConn, it could have been used for other connections, so we need to keep it open.
I think I need to document that behavior in http3.Server.Serve though.

Can you close the net.PacketConn yourself in Caddy, after calling Listener.Close()?

[mholt]

Sorry, I think the highlighted segment I linked to was a bit of a red herring; in other words, mostly (or totally) unrelated to my question. But, I found out that adding s.conn.Close() to the line just before or just after that highlighted segment solved the problem I'm having.

If you haven't seen it yet, I describe an issue with this, however, that I think is worth a look: caddyserver/caddy#2727 (comment) -- since the only thing that seems to trigger it is the activation of debug mode.

You could run an HTTP server and HTTP clients on the same net.PacketConn.

This kinda blows my mind, but it makes sense... so you mean the PacketConn is deliberately not closed, so that any clients which may also be using it can continue to do so?

Hmm 🤔 ...

Can you close the net.PacketConn yourself in Caddy, after calling Listener.Close()?

Do you mean, "after calling http3.Server.Close()"?

Yes, I can... however, there are 2 problems:

1. That adds more bookkeeping for me to do... do you think wanting to close the listener will be so uncommon? Would you consider adding a new method like CloseListener() or ClosePacketConn() to the Server type? (This is arguably a non-issue, but would be convenient!)

2. Perhaps more serious, please see Experimental IETF-standard HTTP/3 support caddyserver/caddy#2727 (comment) as I suspect there is a bug here; I'm unable to get it to work consistently with debug mode off, only when I enable it does it work consistently (and obviously I wouldn't want to do that for production use).

We're almooooost there, though!

[mholt]

To summarize the latest updates on the issue about closing the server:

• The issue I described in Experimental IETF-standard HTTP/3 support caddyserver/caddy#2727 (comment) still persists, but less frequently than it used to (I think, maybe not)
• Not closing the http3.Server seems to work, but I'm guessing that leaks resources. Closing it should be the right thing to do, but doing so causes the issue to emerge.

[marten-seemann]

Hi @mholt, I read through caddyserver/caddy#2727 again, but I'm not really sure I understand what the actual problem is at this point. Can you update me on the issue?

[mholt]

@marten-seemann The most relevant details and instructions are in this comment: caddyserver/caddy#2727 (comment). That comment also has the instructions to reproduce the bug. The latest update at the end of the issue shows that although it once appeared fixed, it actually wasn't, as I was able to reproduce the bug. We work around it by not closing the HTTP/3 server, which seems like a leak.

[marten-seemann]

Hi @mholt, unfortunately I can't run the example any more. Using the caddy.json you posted, I get the following (running Caddy from the v2 branch):

❯ go run main.go run -config caddy.json
2020/02/17 14:52:56.135	INFO	admin	admin endpoint started	{"address": "localhost:2019", "enforce_origin": false, "origins": ["localhost:2019"]}
2020/02/17 21:52:56 [INFO][cache:0xc00014d1d0] Started certificate maintenance routine
2020/02/17 21:52:56 [WARNING] Stapling OCSP: no OCSP stapling for [localhost]: no OCSP server specified in certificate
2020/02/17 14:52:56.136	INFO	tls	cleaned up storage units
2020/02/17 14:52:56.136	INFO	admin	Caddy 2 serving initial configuration

As you can see, Caddy doesn't even listen on the right port, and doesn't seem to use QUIC at all. What am I doing wrong?

[mholt]

@marten-seemann I also have a few updates on my end for you...

First off, Caddy *is* listening on the correct port -- the log entry you're probably referring to is the address of the admin endpoint config API: admin endpoint started {"address": "localhost:2019", ... -- and not the actual HTTP/3 server.

As for serving QUIC, Sorry about the confusion. 😅 I forgot that since I wrote that comment, I made HTTP/3 opt-in (not on by default, because of this issue). I've just updated that config snippet with the fix, which is to add:

"experimental_http3": true

to the "myserver" object (adjacent to "tls_connection_policies" and "routes" and "listen").

I've also updated the curl command that reloads the config, to force a reload even if the config hasn't changed.

Since I've also implemented a workaround since I wrote that post, you'll need to uncomment these lines in Caddy:

https://github.com/caddyserver/caddy/blob/dd103a67876bd19afd000bda4410fbf83ee5b57d/modules/caddyhttp/caddyhttp.go#L329-L336

Then comment these lines, which manually close the HTTP/3 servers' listeners as our workaround:

https://github.com/caddyserver/caddy/blob/dd103a67876bd19afd000bda4410fbf83ee5b57d/modules/caddyhttp/caddyhttp.go#L341-L346

Then perform the instructions again.

I can no longer get the "Handshake did not complete in time" error, but as you continue to reload the config using curl a few times, you'll see this new log line:

[DEBUG] udp/:2443: Usage counter should not go above 2 or maybe 3, is now: 5

This means that when calling Close() on the HTTP/3 server, it is not closing its underlying listener. The second block of code, which should be commented out for this test, is working around this by closing the listeners manually... if you uncomment that code again you'll see that the count doesn't go above 2.

So, a couple of questions I guess:

• Is it intentional that we have to separately close the listeners after closing the servers? If so, isn't that leaking resources?
• Do you get the "Handshake did not complete in time" error anymore? It may not be consistent (it wasn't when we did this in our tests ~6 months ago), so you might have to try a few times. I haven't been able to see it today though after about a dozen tries.

Maybe this is good news, if I can just get your feedback / experience to confirm, then maybe we can close this, finally... maybe.

[marten-seemann]

@mholt Thanks for updating the config files. The first request now goes through successfully.
However, after reloading the config QUIC doesn't work any more, no matter if I'm closing the h3listeners in Caddy or not.

I'm getting the following error when reloading the config:

❯ curl -X POST -d @caddy.json -H "Content-Type: application/json" -H "Cache-Control: must-revalidate" "http://localhost:2019/load"
Warning: Couldn't read data from file "caddy.json", this makes an empty POST.

Is that expected?

[mholt]

Hmm, no, is your config file in the current directory called caddy.json? If not you need to adjust the -d parameter.

[marten-seemann]

My bad, I was running curl from the wrong directory.

Now everything works as expected, no matter if Caddy is closing the h3listeners or not.

Is it intentional that we have to separately close the listeners after closing the servers? If so, isn't that leaking resources?

Yes, since QUIC allows you to run server and client on the same net.PacketConn. So if you give a packet conn to us via http.Server.Serve, we don't know if you're going to use it for a QUIC client (unfortunately, our API currently doesn't allow using it for a HTTP/3 client yet).

[mholt]

Now everything works as expected, no matter if Caddy is closing the h3listeners or not.

That's great! Let's assume for now that it's fixed unless we hear of it cropping up again. I've made a commit that enables closing both the servers and the listeners, and put a comment in the code linking to relevant issues in case the bug ever arises again.

It'd still be nice to have CloseGracefully(ctx) implemented, but I guess that's not super urgent.

Yes, since QUIC allows you to run server and client on the same net.PacketConn. So if you give a packet conn to us via http.Server.Serve, we don't know if you're going to use it for a QUIC client (unfortunately, our API currently doesn't allow using it for a HTTP/3 client yet).

Gotcha, I guess that'll make more sense once the H3 client API is finished then. Might be a good idea to document that in the godoc: https://pkg.go.dev/github.com/lucas-clemente/quic-go@v0.14.4/http3?tab=doc#Server.Close (if you think it is relevant, it is in Caddy's case, anyway).

Thanks!

[mholt]

We're looking at graduating HTTP/3 out of "experimental" status in Caddy and enabling it by default soon. Just FYI. :) It might be nice to have a graceful way to close listeners working, but it doesn't need to be a showstopper, so no pressure there. Just wanted to let you know that we're moving on this soon!