websocket: add reuseport support #2261

chaitanyaprem · 2023-04-26T07:39:06Z

Implemented reuse-port capability for websocket transport #1435 using the existing net/reuseport transport and libp2p/go-reuseport.

In order to utilize REUSEPORT, reuseport option has to be passed while creating the websocket transport.
This would allow for multiple threads/routines to listen on same IP/port combination.

Included a test that would verify reuseport by running 2 go-routines that would listen on same address and accept connections from 4 different clients.

Reran the existing test-suite for the websocket package to ensure no other tests fail.

Note: Tested the new changes in ubuntu-20.04 and windows-11 environment.

marten-seemann

Thank you for your PR! This mostly looks good, just a few comments.

p2p/transport/websocket/websocket.go

p2p/transport/websocket/websocket_test.go

chaitanyaprem · 2023-04-27T14:09:47Z

Thank you for your PR! This mostly looks good, just a few comments.

You are welcome. Pushed new commits addressing your review comments.

p2p/transport/websocket/websocket_test.go

chaitanyaprem · 2023-05-02T05:07:02Z

@marten-seemann please approve the workflows post which this can be merged.

marten-seemann

Thank you @chaitanyaprem! The code looks good to me, but I'm a bit confused with the test case.

Unfortunately, we just switched back to the Gorilla WebSocket library (#2280). This is creating merge conflicts with this PR. Would you mind rebasing this PR on top of the current master?

p2p/transport/websocket/websocket_test.go

chaitanyaprem · 2023-05-16T05:17:57Z

Thank you @chaitanyaprem! The code looks good to me, but I'm a bit confused with the test case.

Unfortunately, we just switched back to the Gorilla WebSocket library (#2280). This is creating merge conflicts with this PR. Would you mind rebasing this PR on top of the current master?

Will resolve and push updated ones.

Listen on unspecified port. Verify that client connections are distributed amongst multiple listeners.

chaitanyaprem · 2023-05-21T01:29:59Z

Pushed updated changes as per review comments.

Maelkum

Nice PR! (commenting as an outsider)

Q: This PR adds the ability to listen on the same port by multiple parties. The TCP implementation for reuseport will also try to use listening ports also for dialing. This is not handled in this PR, is that right?

p2p/transport/websocket/websocket.go

p2p/transport/websocket/websocket_test.go

chaitanyaprem · 2023-05-23T05:06:36Z

Nice PR! (commenting as an outsider)

Q: This PR adds the ability to listen on the same port by multiple parties. The TCP implementation for reuseport will also try to use listening ports also for dialing. This is not handled in this PR, is that right?

Thanks @Maelkum for your comments. Addressed all the others.
You are right that reuseport is not added for dialing as it doesn't seem straight-forward with gorilla websocket.
Will see how to get around that.

chaitanyaprem · 2023-05-26T00:56:45Z

Wow, this is surprising that load balancing across multiple listeners is not functioning on windows as well as Mac even-though 2 sockets are allowed to listen on same address+port.

Wrt Windows, i found this ref which does not guarantee the behaviour of SO_REUSEADDR.
Quote from windows ref is not very clear wrt load balancing.
For example, if all of the sockets on the same port provide TCP service, any incoming TCP connection requests over the port cannot be guaranteed to be handled by the correct socket — the behavior is non-deterministic.

Has the TCP reuse port been tested for load balancing?

Note: While going through the web searching for expected behaviour for these socket options, noticed that for FreeBSD based systems in order for load balancing to be supported an additional option has to be set SO_REUSEPORT_LB, Ref. This is something to be explored and added to the reuseport package

Will update once i investigate further.

chore: reuseport test script-Skipping connection load balancing check for architectures other than linux as it is not gauranteed.

chaitanyaprem · 2023-05-26T10:49:41Z

Update on further investigation, looks like macOS also doesn't support load balancing across listeners.
Few refs:
Ref1
Ref2

So, for now I have skipped checking for load balancing of connections for any platform other than linux in the test script.
Raised an enhancement issue on the go-reuseport repo libp2p/go-reuseport#105 in order to enable LB functionality for FREEBSD based systems by setting SO_REUSEPORT_LB as indicated above.

chaitanyaprem · 2023-05-30T08:29:20Z

Pushed changes to reuse port in case of dailing.
Note that in case of wss with SNI, reuse port cannot be used as the dail function has already been replaced in order to not resolve address and directly do a TCP Dial.

chaitanyaprem · 2023-06-03T05:20:33Z

@marten-seemann @MarcoPolo All required changes and review comments have been addressed.
Please go ahead and merge this PR if nothing else is required.

marten-seemann · 2023-08-14T03:21:14Z

@chaitanyaprem The WebSocket tests are failing on CI. Can you please take a look?

chaitanyaprem · 2023-08-17T01:40:47Z

Surprisingly the test is successful now, i ran it on a local ubuntu vm (Ubuntu 22.04.3 LTS) as well and there also tests are successful.

It is hard to determine whether some other change fixed the issue or is the test just flaky.

marten-seemann · 2023-08-17T08:18:17Z

I reran the test suite and it's failing again. We can't introduce any flaky tests, please make sure it consistently passes (you can run the test suite with -count=1000 locally).

chaitanyaprem · 2023-08-17T08:35:16Z

I reran the test suite and it's failing again. We can't introduce any flaky tests, please make sure it consistently passes (you can run the test suite with -count=1000 locally).

Ran it 1000 times and it fails sometimes.
Commenting the additional load balancing check for now. Looks like linux is doesn't gaurantee it.

p-shahi · 2023-08-25T03:22:58Z

@sukunrt is gonna help take this over the finish line in time for the v0.31 release @chaitanyaprem

sukunrt

I have a couple of other nits, but let's get the dial side to reuse the port we are listening on first.

p2p/transport/websocket/websocket_test.go

sukunrt · 2023-08-25T08:35:16Z

p2p/transport/websocket/listener.go

+		nl, err = reuseport.Listen(lnet, lnaddr)
+		if err != nil {


We want to use the transport's reuseTransport field. Otherwise this will never reuse the port we're listening on when dialing. I guess you can pass them in to newListener.

Also add a test that we are reusing port when dialing.

sukunrt · 2023-08-25T08:36:42Z

p2p/transport/websocket/websocket.go

+		dialer.NetDial = func(network, address string) (net.Conn, error) {
+			return t.reuseTransport.DialContext(ctx, raddr)
+		}
+	}
 	if isWss {
 		sni := ""
 		sni, err = raddr.ValueForProtocol(ma.P_SNI)


This will also need to have the reuse logic: https://github.com/libp2p/go-libp2p/blob/master/p2p/transport/websocket/websocket.go#L205

sukunrt · 2023-09-15T08:23:49Z

@chaitanyaprem just checking in. Let me know if you have any questions here. I'm available in filecoin slack(link in repo README).

chaitanyaprem · 2023-09-25T07:51:13Z

@chaitanyaprem just checking in. Let me know if you have any questions here. I'm available in filecoin slack(link in repo README).

Got little busy with other work and hence not able to take a look at this.
Thanks for providing a channel to reach out, will do once i start working on this again.

sukunrt · 2023-09-26T11:28:52Z

Thanks for the update @chaitanyaprem

Maelkum · 2024-02-08T14:24:38Z

Hi guys! I hope this is not me being rude.. At a similar time when this PR was opened, I played around with implementing a similar thing. I rebased those changes to sync with the latest master (at the time this repo used nhooyr.io/websocket).

Tested it mostly in the listen => dial scenario, compared to multiple listens.

Since this PR is open for a while now, I figured I'd throw the link here. Feel free to cherry pick any piece of code out of there if it's useful. Hope this isn't bad manners!

Maelkum#1

sukunrt · 2024-02-21T14:30:24Z

Thanks @Maelkum. This PR has been dormant for a while so I think it's okay. Do you want to open a new PR or take over this one?
@chaitanyaprem is it fine if @Maelkum continues the work here?

chaitanyaprem · 2024-02-21T15:00:23Z

Thanks @Maelkum. This PR has been dormant for a while so I think it's okay. Do you want to open a new PR or take over this one? @chaitanyaprem is it fine if @Maelkum continues the work here?

Thanks @Maelkum
Feel free to takeover, have not been able to get to this one myself.

Maelkum · 2024-02-24T16:03:04Z

@sukunrt @chaitanyaprem
Thanks guys! I'll open a new PR and link this one there, I think it will be tidier that way.

chaitanyaprem added 3 commits April 26, 2023 11:08

feat: added REUSEPORT support for websocket listeners libp2p#1435

e7b4454

Added support for using reuseport in connection Dialing libp2p#1435

445f860

chore: cleaned-up the websocket reuse test

4279f92

marten-seemann self-requested a review April 26, 2023 14:58

chaitanyaprem added 2 commits April 27, 2023 05:24

chore: addressed review comments for libp2p#2261

80b3b34

chore: address linter error in websocket test script

804ba36

marten-seemann reviewed Apr 27, 2023

View reviewed changes

p2p/transport/websocket/websocket.go Outdated Show resolved Hide resolved

p2p/transport/websocket/websocket.go Outdated Show resolved Hide resolved

p2p/transport/websocket/websocket_test.go Outdated Show resolved Hide resolved

marten-seemann linked an issue Apr 27, 2023 that may be closed by this pull request

websocket: use reuseport #1435

Open

marten-seemann changed the title ~~feat: add reuseport support for websocket transport~~ websocket: add reuseport support Apr 27, 2023

MarcoPolo reviewed Apr 27, 2023

View reviewed changes

p2p/transport/websocket/websocket_test.go Outdated Show resolved Hide resolved

chaitanyaprem requested review from marten-seemann and MarcoPolo May 7, 2023 06:52

marten-seemann reviewed May 15, 2023

View reviewed changes

p2p/transport/websocket/websocket_test.go Outdated Show resolved Hide resolved

p2p/transport/websocket/websocket_test.go Outdated Show resolved Hide resolved

Merge branch 'master' into feat/websocket-reuseport

f73c1ba

chaitanyaprem added 2 commits May 21, 2023 06:56

chore: addressed review comments wrt test.

cc83e5a

Listen on unspecified port. Verify that client connections are distributed amongst multiple listeners.

Merge branch 'libp2p:master' into feat/websocket-reuseport

9c539a6

Maelkum reviewed May 22, 2023

View reviewed changes

chore: addressed review comments and cleanedup test script

153829a

Update websocket_test.go

07aae9b

chore: reuseport test script-Skipping connection load balancing check for architectures other than linux as it is not gauranteed.

feat: use reuseport wihle dialing client connections if set

ad59795

chaitanyaprem requested a review from marten-seemann May 30, 2023 05:22

Merge branch 'master' into feat/websocket-reuseport

b170b43

sukunrt mentioned this pull request Aug 7, 2023

go-libp2p v0.30 #2428

Closed

21 tasks

Merge branch 'master' into feat/websocket-reuseport

863f51d

marten-seemann mentioned this pull request Aug 15, 2023

go-libp2p v0.31 #2481

Closed

23 tasks

chaitanyaprem added 2 commits August 16, 2023 18:51

Merge branch 'libp2p:master' into feat/websocket-reuseport

4cccef8

Merge branch 'libp2p:master' into feat/websocket-reuseport

4179d80

chore:comment lb check in reuseport test to avoid flaky results

8840f19

chore: fix gofmt error

f660226

sukunrt requested changes Aug 25, 2023

View reviewed changes

p-shahi mentioned this pull request Aug 31, 2023

go-libp2p v0.32 #2523

Closed

25 tasks

chaitanyaprem added 2 commits September 6, 2023 11:19

Merge branch 'master' into feat/websocket-reuseport

3fea95c

chore: increase number of clients to avoid flakiness of reuseport test

c01a725

marten-seemann added the need/author-input Needs input from the original author label Oct 20, 2023

Maelkum mentioned this pull request Feb 24, 2024

Add reuseport functionality to websocket protocol (v2) #2719

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

websocket: add reuseport support #2261

websocket: add reuseport support #2261

chaitanyaprem commented Apr 26, 2023 •

edited

Loading

marten-seemann left a comment

chaitanyaprem commented Apr 27, 2023

chaitanyaprem commented May 2, 2023

marten-seemann left a comment

chaitanyaprem commented May 16, 2023

chaitanyaprem commented May 21, 2023

Maelkum left a comment

chaitanyaprem commented May 23, 2023

chaitanyaprem commented May 26, 2023

chaitanyaprem commented May 26, 2023 •

edited

Loading

chaitanyaprem commented May 30, 2023 •

edited

Loading

chaitanyaprem commented Jun 3, 2023

marten-seemann commented Aug 14, 2023

chaitanyaprem commented Aug 17, 2023

marten-seemann commented Aug 17, 2023

chaitanyaprem commented Aug 17, 2023

p-shahi commented Aug 25, 2023

sukunrt left a comment

sukunrt Aug 25, 2023

sukunrt Aug 25, 2023

sukunrt commented Sep 15, 2023

chaitanyaprem commented Sep 25, 2023

sukunrt commented Sep 26, 2023

Maelkum commented Feb 8, 2024

sukunrt commented Feb 21, 2024

chaitanyaprem commented Feb 21, 2024

Maelkum commented Feb 24, 2024

websocket: add reuseport support #2261

Are you sure you want to change the base?

websocket: add reuseport support #2261

Conversation

chaitanyaprem commented Apr 26, 2023 • edited Loading

marten-seemann left a comment

Choose a reason for hiding this comment

chaitanyaprem commented Apr 27, 2023

chaitanyaprem commented May 2, 2023

marten-seemann left a comment

Choose a reason for hiding this comment

chaitanyaprem commented May 16, 2023

chaitanyaprem commented May 21, 2023

Maelkum left a comment

Choose a reason for hiding this comment

chaitanyaprem commented May 23, 2023

chaitanyaprem commented May 26, 2023

chaitanyaprem commented May 26, 2023 • edited Loading

chaitanyaprem commented May 30, 2023 • edited Loading

chaitanyaprem commented Jun 3, 2023

marten-seemann commented Aug 14, 2023

chaitanyaprem commented Aug 17, 2023

marten-seemann commented Aug 17, 2023

chaitanyaprem commented Aug 17, 2023

p-shahi commented Aug 25, 2023

sukunrt left a comment

Choose a reason for hiding this comment

sukunrt Aug 25, 2023

Choose a reason for hiding this comment

sukunrt Aug 25, 2023

Choose a reason for hiding this comment

sukunrt commented Sep 15, 2023

chaitanyaprem commented Sep 25, 2023

sukunrt commented Sep 26, 2023

Maelkum commented Feb 8, 2024

sukunrt commented Feb 21, 2024

chaitanyaprem commented Feb 21, 2024

Maelkum commented Feb 24, 2024

chaitanyaprem commented Apr 26, 2023 •

edited

Loading

chaitanyaprem commented May 26, 2023 •

edited

Loading

chaitanyaprem commented May 30, 2023 •

edited

Loading