quic: Update to quic-go v0.36.2 #2424

MarcoPolo · 2023-07-11T19:47:29Z

quic-go changed its API in v0.35, this PR updates our usage.

Most of the changes in this PR are due to renaming things from conn -> transport. Previously we could only share a underlying socket by sharing the "connection" between quic-go dialers/listeners. quic-go v0.35.0 introduces a Transport that represents QUIC on a socket. This Transport is what is shared between different dialers/listeners to reuse the same socket. I think that's a better abstraction since it's clear when something is getting shared vs not.

A lot of the renaming is to make clear what is happening. For example, reuse.Listen is now called reuse.TransportForListen since what we actually get back from the call is a quic.Transport. Similarly most things that were previously referencing a "conn" (as in packetconn) now reference a transport. Hopefully this also clears up any confusion about different "connection" like objects.

sukunrt

I don't think we should rename like

reuse.Listen is now called reuse.TransportForListen

For our purposes the PacketConn is now replaced by Transport. This renaming makes things clearer right now but given that this will be the API of quic-go going forward, these names will feel out of place 2-3 months from now.
The type system already provides the type of the return value so it's clear what sort of object we are getting. Plus most(all?) of these are internal to transport/quic so the expected type of object is also clear from the context.

IMO we should just add a long explanation in the changelog / commit message.

p2p/transport/quicreuse/reuse.go

p2p/transport/quicreuse/connmgr.go

p2p/transport/quicreuse/reuse.go

MarcoPolo · 2023-07-12T17:33:35Z

I don't think we should rename like

reuse.Listen is now called reuse.TransportForListen

For our purposes the PacketConn is now replaced by Transport. This renaming makes things clearer right now but given that this will be the API of quic-go going forward, these names will feel out of place 2-3 months from now.

What do you mean by the names will feel out of place? As long as quic-go gives us a transport and as long as we are getting a Transport for listening from this method, the new name makes sense.

The type system already provides the type of the return value so it's clear what sort of object we are getting. Plus most(all?) of these are internal to transport/quic so the expected type of object is also clear from the context.

It's really confusing for a Listen method to not return a listener, just like it's confusing for Dial to not return a connection to another node. This makes the API clearer because you aren't getting a Listener, you're getting the thing that can Listen (Transport).

IMO we should just add a long explanation in the changelog / commit message.

While better than nothing, I think this isn't enough. We are already breaking the API by no longer returning a PacketConn we should update the API to make it clear what we are doing and returning. A method called Listen should return a Listener-like thing that you can call Accept on.

I'm open to changing the name from TransportForListen to something else as long as it's not Listen. Maybe ListenQuicTransport? It would be in line with Go std's ListenTCP/ListenUDP/ListenPacket/ListenX

marten-seemann · 2023-07-12T18:08:46Z

quic-go v0.36.2 just shipped: https://github.com/quic-go/quic-go/releases/tag/v0.36.2.

sukunrt · 2023-07-12T18:38:55Z

A method called Listen should return a Listener-like thing that you can call Accept on.

I see your point. Thanks for the explanation.
My point was the pConn had the same interface as transport and the method wasn't named pConnForListen. But maybe it should have.

These names will feel out of place 2-3 months from now.

I meant Transport in the name was redundant with the returned type

But I am convinced about the renaming. I suggest we either keep it as is(the new names, TransporForListen and TransportForDial) or rename struct reuse to reuseTransport and name the methods ForListen and ForDial so it reads like reuseTransport.ForListen

I do not like naming along the lines ListenTCP because the returned object is a listener of type TCP which doesn't fit in with what we're doing. So TransportForListen is better.

MarcoPolo · 2023-07-13T01:28:39Z

p2p/transport/quicreuse/connmgr_test.go

-		conn = c
-		return nil, errors.New("listen error")
+	if runtime.GOOS == "windows" {
+		t.Skip("skipping on windows. Not sure why this fails")


@marten-seemann Any clue off the top of your head why this fails? https://github.com/libp2p/go-libp2p/actions/runs/5537794470/jobs/10107022242#step:8:4371.

Presumably this means that on windows after calling OptimizeConn the returned conn isn't a (quic.OOBCapablePacketConn)

Yes, that’s right. It’s using a basicConn on Windows: https://github.com/quic-go/quic-go/blob/418b866e3260b65496eb0d4420465e9d11b07e5f/sys_conn.go#L104

I’m becoming more and more convinced that OptimizeConn is not the right API. We should change that in a future release.

Yeah. I think if Transport has a WriteTo that would be better. The problem is I can't treat the conn from OptimizeConn as a normal PacketConn once I give it to the Transport.

We'll make that change in the v0.37 release (which drops support for Go 1.19, so we won't be able to use that release until mid August).

Do you see any blockers for this PR?

No, this is fine for now. And the change will be very straightforward

sukunrt

Just a couple of nits.

p2p/transport/quicreuse/reuse.go

p2p/transport/webtransport/transport.go

sukunrt

This is nicer. Thanks. I think this one comment is the last renaming left.

sukunrt · 2023-07-13T19:19:31Z

p2p/transport/quicreuse/reuse.go

+	// Used to send packets directly around QUIC. Useful for hole punching.
+	WriteTo([]byte, net.Addr) (int, error)


p2p/transport/quicreuse/connmgr.go

* Update go.mod files * Update to new quic-go API * More renaming * Add test back in * Workaround quic-go#3947 * Fix transitive dep * Use own pointer to packetConn * Update to quic-go v0.36.2 * Remove workaround * Embed quic.Transport * Downgrade qtls-go1-20 * Rename ConnManager.metricsTracer to mt * Close transport after test ends * Close conn when transport closes * Return better error * Avoid conflicts with parallel tests * Skip conn assert on windows * Add metrics tracer back in * Don't use tracers here * Add comment to WriteTo * Finish renaming conn -> transport where appropriate * Back out unrelated change * One more rename

MarcoPolo added 2 commits July 11, 2023 12:32

Update go.mod files

f7f17a1

Update to new quic-go API

6c6c541

MarcoPolo marked this pull request as draft July 11, 2023 19:47

MarcoPolo added 3 commits July 11, 2023 20:38

More renaming

cda4ebc

Add test back in

511bb13

Workaround quic-go#3947

291f150

MarcoPolo requested a review from sukunrt July 12, 2023 03:55

MarcoPolo added 2 commits July 11, 2023 20:58

Fix transitive dep

843c17c

Use own pointer to packetConn

3c2e7c4

sukunrt reviewed Jul 12, 2023

View reviewed changes

p2p/transport/quicreuse/reuse.go Outdated Show resolved Hide resolved

MarcoPolo added 2 commits July 12, 2023 11:22

Update to quic-go v0.36.2

688fbc9

Remove workaround

6f1d91c

MarcoPolo added 8 commits July 12, 2023 11:39

Embed quic.Transport

4dc8399

Downgrade qtls-go1-20

1fa604a

Rename ConnManager.metricsTracer to mt

b8d768a

Close transport after test ends

d1ad906

Close conn when transport closes

efe0d08

Return better error

c49d3d9

Avoid conflicts with parallel tests

ee672cb

Skip conn assert on windows

d0c0cf9

MarcoPolo commented Jul 13, 2023

View reviewed changes

MarcoPolo marked this pull request as ready for review July 13, 2023 01:28

MarcoPolo added 2 commits July 13, 2023 09:24

Add metrics tracer back in

eb830c1

Don't use tracers here

69bf883

MarcoPolo changed the title ~~quic: Update to quic-go v0.36.1~~ quic: Update to quic-go v0.36.2 Jul 13, 2023

sukunrt approved these changes Jul 13, 2023

View reviewed changes

p2p/transport/quicreuse/reuse.go Show resolved Hide resolved

p2p/transport/quicreuse/reuse.go Outdated Show resolved Hide resolved

p2p/transport/webtransport/transport.go Outdated Show resolved Hide resolved

MarcoPolo added 3 commits July 13, 2023 11:38

Add comment to WriteTo

a451cfa

Finish renaming conn -> transport where appropriate

7773888

Back out unrelated change

0f7e912

sukunrt approved these changes Jul 13, 2023

View reviewed changes

One more rename

b632427

MarcoPolo merged commit 8a60a68 into master Jul 13, 2023

MarcoPolo mentioned this pull request Jul 14, 2023

go-libp2p v0.29 #2326

Closed

29 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

quic: Update to quic-go v0.36.2 #2424

quic: Update to quic-go v0.36.2 #2424

MarcoPolo commented Jul 11, 2023 •

edited

Loading

sukunrt left a comment •

edited

Loading

MarcoPolo commented Jul 12, 2023 •

edited

Loading

marten-seemann commented Jul 12, 2023

sukunrt commented Jul 12, 2023 •

edited

Loading

MarcoPolo Jul 13, 2023

marten-seemann Jul 13, 2023

MarcoPolo Jul 13, 2023

marten-seemann Jul 13, 2023

MarcoPolo Jul 13, 2023

sukunrt left a comment

sukunrt left a comment

sukunrt Jul 13, 2023

		// Used to send packets directly around QUIC. Useful for hole punching.
		WriteTo([]byte, net.Addr) (int, error)

quic: Update to quic-go v0.36.2 #2424

quic: Update to quic-go v0.36.2 #2424

Conversation

MarcoPolo commented Jul 11, 2023 • edited Loading

sukunrt left a comment • edited Loading

Choose a reason for hiding this comment

MarcoPolo commented Jul 12, 2023 • edited Loading

marten-seemann commented Jul 12, 2023

sukunrt commented Jul 12, 2023 • edited Loading

MarcoPolo Jul 13, 2023

Choose a reason for hiding this comment

marten-seemann Jul 13, 2023

Choose a reason for hiding this comment

MarcoPolo Jul 13, 2023

Choose a reason for hiding this comment

marten-seemann Jul 13, 2023

Choose a reason for hiding this comment

MarcoPolo Jul 13, 2023

Choose a reason for hiding this comment

sukunrt left a comment

Choose a reason for hiding this comment

sukunrt left a comment

Choose a reason for hiding this comment

sukunrt Jul 13, 2023

Choose a reason for hiding this comment

MarcoPolo commented Jul 11, 2023 •

edited

Loading

sukunrt left a comment •

edited

Loading

MarcoPolo commented Jul 12, 2023 •

edited

Loading

sukunrt commented Jul 12, 2023 •

edited

Loading