Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: WebRTC reuse QUIC conn #2889

Merged
merged 3 commits into from
Aug 1, 2024
Merged

Conversation

MarcoPolo
Copy link
Collaborator

This lets users listen on the same port for WebRTC and QUIC. Kubo folks asked for this as it would be very beneficial to their deployment.

One open question for me is:

  • Do we need to make sure this works well with the Observed Address Manager? I think it should just work, but we should double check. Ideally we should know our public webrtc-direct address if we've discovered our public quic address when they use the same port.

@MarcoPolo MarcoPolo requested a review from sukunrt July 30, 2024 04:47
@sukunrt
Copy link
Member

sukunrt commented Jul 30, 2024

Do we need to make sure this works well with the Observed Address Manager? I think it should just work, but we should double check. Ideally we should know our public webrtc-direct address if we've discovered our public quic address when they use the same port.

We should infer the address from the quic address like we do for webtransport. It'll work seamlessly once we have the same logic as inferWebtransportAddrsFromQuic

This is more useful for webrtc than for other transports as a server will never dial /webrtc-direct. It will almost always prefer quic or webtransport or tcp. With this PR, we can infer our publicly visible address from our quic address.

p2p/net/swarm/swarm_listen.go Show resolved Hide resolved
p2p/transport/quicreuse/connmgr.go Show resolved Hide resolved
p2p/transport/quicreuse/nonquic_packetconn.go Show resolved Hide resolved
p2p/transport/webrtc/transport.go Show resolved Hide resolved
lidel added a commit to ipfs/kubo that referenced this pull request Jul 30, 2024
lidel added a commit to ipfs/kubo that referenced this pull request Jul 30, 2024
lidel added a commit to ipfs/kubo that referenced this pull request Jul 30, 2024
lidel added a commit to ipfs/kubo that referenced this pull request Jul 30, 2024
lidel added a commit to ipfs/kubo that referenced this pull request Jul 30, 2024
@MarcoPolo
Copy link
Collaborator Author

Discussed out of band about changes to make this order agnostic with the QUIC transport, since, as it is implemented in this PR, the QUIC listener starts first. Sukun and I agreed the existing logic is good enough. Which is to sort the listeners and start the QUIC listener first (if there is one). Summary of the points we discussed:

  • To reuse the same port, WebRTC has to go through a QUIC transport.
  • It doesn't make sense to always instantiate a QUIC transport when setting up a WebRTC listener.
    • It makes WebRTC unnecessarily depend on a QUIC transport being present.
    • Can lead to hard to debug issues. And cause surprising behavior.
  • It's much easier to depend on a QUIC transport that has already been started from a prior QUIC listener.
  • Sorting the listeners before starting is relatively easy and straightforward.

lidel
lidel previously requested changes Jul 30, 2024
Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for pushing this forward, reusing same port has big impact for DX/UX, allowing us faster rollout with less hiccups.

Took it for a spin in ipfs/kubo#10463 and fx seems to have issue with libp2pwebrtc.ListenUDPFn.

To reproduce, rebase this PR on top of #2887, to ensure WebRTC Direct is enabled by default, then run go test ./...: fx errors with missing type: libp2pwebrtc.ListenUDPFn (same error as in https://github.com/ipfs/kubo/pull/10463/files#r1697532128).

@MarcoPolo
Copy link
Collaborator Author

We should infer the address from the quic address like we do for webtransport. It'll work seamlessly once we have the same logic as inferWebtransportAddrsFromQuic

I don't think we actually need this anymore. I've made a PR to remove that function here #2892.

@MarcoPolo
Copy link
Collaborator Author

Took it for a spin in ipfs/kubo#10463 and fx seems to have issue with libp2pwebrtc.ListenUDPFn.

That was my mistake sorry. I think it was complaining that it couldn't create the WebRTC transport for the AutoNAT host because I didn't provide it to it (only for the basic host).

@MarcoPolo MarcoPolo force-pushed the marco/webrtc-reuse-quic-conn branch from b3e96ef to dfd9ec8 Compare July 31, 2024 21:10
Copy link
Member

@sukunrt sukunrt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!
Thanks @MarcoPolo 🎉

@MarcoPolo MarcoPolo merged commit db41da3 into master Aug 1, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants