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.

Sign up for GitHub

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

Jump to bottom

fix(quic): allow listening on ipv4 and ipv6 separately #4289

Merged
merged 11 commits into from
Aug 8, 2023
Merged

fix(quic): allow listening on ipv4 and ipv6 separately #4289

merged 11 commits into from
Aug 8, 2023

Conversation

jxs
Copy link
Member

@jxs jxs commented Aug 3, 2023 •
edited by thomaseizinger
Loading

Description

Resolves #4165.

Notes & open questions

Should we also add a test for this?

Change checklist

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • A changelog entry has been made in the appropriate crates

@jxs jxs changed the title fix(quic) allow listening on listen on ipv4 and v6 fix(quic): allow listening on listen on ipv4 and v6 Aug 3, 2023
@thomaseizinger thomaseizinger changed the title fix(quic): allow listening on listen on ipv4 and v6 fix(quic): allow listening on ipv4 and ipv6 seperately Aug 3, 2023
thomaseizinger
thomaseizinger approved these changes Aug 3, 2023
View reviewed changes
Copy link
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

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

Thanks @jxs!

transports/quic/src/transport.rs Outdated Show resolved Hide resolved
transports/quic/src/transport.rs Outdated Show resolved Hide resolved
transports/quic/Cargo.toml Outdated Show resolved Hide resolved
thomaseizinger
thomaseizinger reviewed Aug 4, 2023
View reviewed changes
Copy link
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

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

Also needs a changelog entry. Is there an easy way to test this? I think attempting to listen on the same port should do it right? We don't actually have to establish connections so it should be a fairly small test :)

transports/quic/src/transport.rs Outdated Show resolved Hide resolved
jxs
jxs commented Aug 4, 2023
View reviewed changes
Copy link
Member Author

@jxs jxs left a comment

Choose a reason for hiding this comment

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

Also needs a changelog entry. Is there an easy way to test this? I think attempting to listen on the same port should do it right? We don't actually have to establish connections so it should be a fairly small test :)

Thanks, updated changelog and added a test ptal Thomas.

transports/quic/Cargo.toml Outdated Show resolved Hide resolved
transports/quic/src/transport.rs Outdated Show resolved Hide resolved
@thomaseizinger thomaseizinger changed the title fix(quic): allow listening on ipv4 and ipv6 seperately fix(quic): allow listening on ipv4 and ipv6 separately Aug 6, 2023
thomaseizinger
thomaseizinger reviewed Aug 6, 2023
View reviewed changes
Copy link
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

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

A few more things, otherwise LGTM!

Thanks @jxs! :)

transports/quic/Cargo.toml Show resolved Hide resolved
transports/quic/Cargo.toml Outdated Show resolved Hide resolved
transports/quic/src/transport.rs Show resolved Hide resolved
mxinden
mxinden reviewed Aug 7, 2023
View reviewed changes
Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

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

Small suggestion. Otherwise looks good to me. Thank you @jxs.

transports/quic/src/transport.rs Outdated Show resolved Hide resolved
@mxinden mxinden added the send-it label Aug 7, 2023
@thomaseizinger
Copy link
Contributor

@jxs The lockfile doesn't seem to be up-to-date.

@jxs
Copy link
Member Author

jxs commented Aug 8, 2023

Thanks for the ping Thomas, updated!

@mergify mergify bot merged commit 02dc432 into libp2p:master Aug 8, 2023
66 of 67 checks passed
@jxs
Copy link
Member Author

jxs commented Aug 10, 2023 •
edited
Loading

can we release 0.9.1-alpha @mxinden? I can then submit a PR to your rust-libp2p-server enabling it

@mxinden
Copy link
Member

mxinden commented Aug 10, 2023

can we release 0.9.1-alpha @mxinden?

Just created #4312.

I can then submit a PR to your rust-libp2p-server enabling it

No need. Will be included in the monorepo soon. See #4311.

@jxs jxs mentioned this pull request Aug 14, 2023
Merged
4 tasks
mergify bot pushed a commit that referenced this pull request Aug 18, 2023
@jxs
refactor(tcp): reducing branching in Transport::create_socket
bf7fe68 
Following #4289 (comment), hereby is the PR to also improve the `create_socket`  using [`for_addr`](https://docs.rs/socket2/latest/socket2/struct.Domain.html#method.for_address). We also add a test for listening on IPv4 and IPv6 separately.

Pull-Request: #4328.
thomaseizinger pushed a commit that referenced this pull request Aug 20, 2023
@jxs @thomaseizinger
fix(quic): allow listening on ipv4 and ipv6 separately
097794d 
Resolves #4165.

Pull-Request: #4289.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thomaseizinger thomaseizinger thomaseizinger approved these changes

@mxinden mxinden mxinden approved these changes

Assignees
No one assigned
Labels
send-it
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

quic: ability to listen on ipv4 and v6
3 participants
@jxs @thomaseizinger @mxinden