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

Add reuseport functionality to websocket protocol (v2) #2719

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

Maelkum
Copy link

@Maelkum Maelkum commented Feb 24, 2024

This PR adds the reuseport functionality for websocket protocol.

The TestReusePortOnDial tests this implementation by starting to listen on a random port, then dialing another endpoint and verifying that the dial used the same port we were previously listening on.

TestReusePortOnListen starts two listeners and twenty dialers and (on Linux) verifies that requests were distributed among both listeners; on Windows and MacOS we can't make such guarantees, so we just verify that everything worked, and all requests were indeed handled.

This PR offers an alternative implementation and operates in the same space as #2261.
Kudos to @chaitanyaprem for his work, I have implemented his documentation references and was inspired by his test for multiple listeners and connection distribution.

The tests for websocket implementation are somewhat noisy (on master too), I don't think warnings like those below stem from these changes:

2024/02/24 17:11:16 http: TLS handshake error from 127.0.0.1:51918: remote error: tls: bad certificate
2024/02/24 17:11:16 http: TLS handshake error from 127.0.0.1:39826: remote error: tls: bad certificate
2024/02/24 17:11:17 http: TLS handshake error from 127.0.0.1:52002: read tcp4 127.0.0.1:38471->127.0.0.1:52002: use of closed network connection
2024/02/24 17:11:17 http: TLS handshake error from 127.0.0.1:34080: read tcp4 127.0.0.1:46259->127.0.0.1:34080: use of closed network connection
2024/02/24 17:11:17 http: TLS handshake error from 127.0.0.1:38720: remote error: tls: bad certificate
2024/02/24 17:11:18 http: TLS handshake error from 127.0.0.1:60538: remote error: tls: bad certificate

"github.com/libp2p/go-reuseport"
)

func reuseportIsAvailable() bool {
Copy link
Author

Choose a reason for hiding this comment

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

Right now this is just overhead - I added it for parity with the TCP implementation that includes checking of a LIBP2P_TCP_REUSEPORT environment variable to change the setting.

Comment on lines +84 to +89
func EnableReuseport() Option {
return func(t *WebsocketTransport) error {
t.enableReuseport = true
return nil
}
}
Copy link
Author

Choose a reason for hiding this comment

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

This is the opposite option from the TCP implementation - there we have it enabled by default, here it needs to be explicitly enabled. I figured it might be better to go the more conservative route.

Comment on lines +638 to +643
conn, err := listener.Accept()
if err != nil {
// Stop condition - this happens when the listener is closed.
require.ErrorIs(t, err, transport.ErrListenerClosed)
return
}
Copy link
Author

Choose a reason for hiding this comment

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

It's possible to just start listeners above and defer Close() calls to it. Then when the test function completes and execution "falls off the end" the listener.Close() call will unblock this and Accept() call will fail with an error.

Still, I think I prefer that one can see what will happen during the test just by reading it. To me it's better than "something will happen after the test is done too". This is a bit ugly but explicit. I'm open to any criticism :)

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.

1 participant