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

quic: add DisableReuseport option #1476

Merged
merged 3 commits into from
Aug 19, 2022
Merged

quic: add DisableReuseport option #1476

merged 3 commits into from
Aug 19, 2022

Conversation

gfanton
Copy link
Contributor

@gfanton gfanton commented May 4, 2022

Moved from libp2p/go-libp2p-quic-transport#272

Fixes #1428

Hey, I just started to add the DisableReuseport options to quic transport. It seems to be working as expected with some cellular operators that couldn't maintain the connection (I tried with this script https://github.com/gfanton/libp2p-reuseport-test)

Just some few things:

  • It seems that there is no option/configuration mechanism, so I added one. Let me know if you are ok with this or if you want to use another method to pass the argument.
  • I wrapped all the conn_tests with a simple table test case to test everything conn related with the reuseport_on/off
  • I have to use net.ListenUDP instead of net.DialUDP for the Dial method to work, otherwise the call to the WriteTo method fails.

Also there are two tests that currently fail with the reuseport disabled:

TestHolePunching

=== RUN   TestHolePunching/reuseport_off
    conn_test.go:59: using a Secp256k1 key: 16Uiu2HAmPxEseg6Hti88hpHTyUB3tQk9bw7paj9yS3rPkBgPnWgM
    conn_test.go:59: using a Ed25519 key: 12D3KooWAaU8sc9j783H9hH3eG3yyJDnvz9Le1LdhCQFbYe5gFyu
    conn_test.go:581: 
        	Error Trace:	conn_test.go:581
        	           				asm_arm64.s:1133
        	Error:      	An error is expected but got nil.
        	Test:       	TestHolePunching/reuseport_off
        	Messages:   	didn't expect to accept any connections
    conn_test.go:602: 
        	Error Trace:	conn_test.go:602
        	Error:      	Condition never satisfied
        	Test:       	TestHolePunching/reuseport_off

TestStatelessReset

=== RUN   TestStatelessReset/reuseport_off
    conn_test.go:59: using a ECDSA key: QmRoF9AeMPnxYMrErq91y2xCvjiULp9n9cJuVwDHu9Kdao
    conn_test.go:59: using a RSA key: QmNvwzR7VDFy1wFdHE3PgV8mCztJbFWz1d5NXVL3wXYVgX
    conn_test.go:534: 
        	Error Trace:	conn_test.go:66
        	           				conn_test.go:534
        	Error:      	Received unexpected error:
        	           	listen udp4 127.0.0.1:55213: bind: address already in use
        	Test:       	TestStatelessReset/reuseport_off

Do these tests make sense when used with resuseport disabled?

@gfanton gfanton force-pushed the feat/quic-disable-reuseport branch from 1365b5e to 0173e41 Compare May 5, 2022 08:38
@marten-seemann marten-seemann self-requested a review May 5, 2022 08:50
@marten-seemann
Copy link
Contributor

The QUIC tests were very flaky, and we've cleaned them up since. Could you rebase this PR on the current master?

@gfanton gfanton force-pushed the feat/quic-disable-reuseport branch from 0173e41 to e88abfa Compare June 8, 2022 14:10
@gfanton gfanton force-pushed the feat/quic-disable-reuseport branch from e88abfa to 8c5b669 Compare August 11, 2022 13:08
@gfanton gfanton marked this pull request as ready for review August 11, 2022 13:08
@gfanton
Copy link
Contributor Author

gfanton commented Aug 11, 2022 •
edited
Loading

I just rebased the branch on master, it seems that all the concerned tests pass now!

marten-seemann
marten-seemann reviewed Aug 12, 2022
View reviewed changes
Copy link
Contributor

@marten-seemann marten-seemann left a comment

Choose a reason for hiding this comment

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

This mostly looks good. I added a few comments.

p2p/transport/quic/conn_test.go
require.Equal(t, serverConn.RemotePeer(), clientID)
require.True(t, serverConn.RemotePublicKey().Equals(clientKey.GetPublic()), "remote public key doesn't match")
}
for _, tc := range connTestCases {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please introduce a separate function, so you don't have to indent everything here.

p2p/transport/quic/conn_test.go
}

func TestResourceManagerSuccess(t *testing.T) {
serverID, serverKey := createPeer(t)
clientID, clientKey := createPeer(t)
for _, tc := range connTestCases {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here.

p2p/transport/quic/options.go Outdated

type Option func(opts *Config) error

type Config struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't need to be exported.

p2p/transport/quic/transport.go Outdated
if err != nil {
return nil, err
}
return reuse.Listen(network, laddr)
return newNoReuseConn(conn), nil
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: This doesn't need a constructor, just use noreuseConn{Conn: conn}.

@marten-seemann marten-seemann added the need/author-input Needs input from the original author label Aug 12, 2022
@gfanton gfanton force-pushed the feat/quic-disable-reuseport branch from 8327714 to f202713 Compare August 16, 2022 14:11
@gfanton gfanton force-pushed the feat/quic-disable-reuseport branch from f202713 to eacccb5 Compare August 16, 2022 14:18
@gfanton
fix: TestStatelessReset/reuseport_off test
93bf6d6 
close listener underlying connection when reuseport is disabled and the close
method called

Signed-off-by: gfanton <[email protected]>
@gfanton gfanton force-pushed the feat/quic-disable-reuseport branch from eacccb5 to 93bf6d6 Compare August 16, 2022 14:47
@gfanton
Copy link
Contributor Author

gfanton commented Aug 16, 2022 •
edited
Loading

I've made the requested changes, but I've missed some DisableReuseport option in some test and still have the same issues described above with TestHolePunching & TestStatelessReset:

  • for TestStatelessReset, I managed to fix it in my last commit by closing the connection with listener.Close releasing the used port.
  • for TestHolePunching I'm still facing the same issue, but I'm not really sure that this test has any interest with reuseport disabled. Do you have any clue about this ?

thanks !

@marten-seemann
Copy link
Contributor

Hole punching is only expected to work with reuseport enabled. You can disable that test for the non-reuseport version (please add a comment why!).

@gfanton
Copy link
Contributor Author

gfanton commented Aug 16, 2022

done 👍

@gfanton gfanton requested a review from marten-seemann August 19, 2022 09:04
@gfanton
Copy link
Contributor Author

gfanton commented Aug 19, 2022

One test is failing, but it doesn't seem to be related to my PR.

@marten-seemann marten-seemann changed the title feat: add DisableReuseport option to quic-transport quic: add DisableReuseport option Aug 19, 2022
@marten-seemann marten-seemann mentioned this pull request Aug 19, 2022
Closed
21 tasks
@marten-seemann marten-seemann removed the need/author-input Needs input from the original author label Aug 19, 2022
@marten-seemann marten-seemann merged commit 0f4a969 into libp2p:master Aug 19, 2022
@gfanton gfanton deleted the feat/quic-disable-reuseport branch August 19, 2022 11:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marten-seemann marten-seemann marten-seemann approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

quic: make the reuseport feature optional
2 participants
@gfanton @marten-seemann