go-libp2p icon indicating copy to clipboard operation
go-libp2p copied to clipboard

feat: add `DisableReuseport` option to quic-transport

Open gfanton opened this issue 3 years ago • 1 comments

Moved from https://github.com/libp2p/go-libp2p-quic-transport/pull/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 avatar May 04 '22 16:05 gfanton

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

marten-seemann avatar May 26 '22 11:05 marten-seemann

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

gfanton avatar Aug 11 '22 13:08 gfanton

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 !

gfanton avatar Aug 16 '22 14:08 gfanton

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!).

marten-seemann avatar Aug 16 '22 15:08 marten-seemann

done 👍

gfanton avatar Aug 16 '22 16:08 gfanton

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

gfanton avatar Aug 19 '22 09:08 gfanton