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

websocket: Replace gorilla websocket transport with nhooyr websocket transport #1982

Merged
merged 5 commits into from
Feb 7, 2023

Conversation

MarcoPolo
Copy link
Collaborator

@MarcoPolo MarcoPolo commented Jan 5, 2023

fixes #1970

  • Run this on the kubo sharness tests and see if anything comes up.
  • Test this with the test-plans interop tester.
  • If that looks okay, ping @thibmeu since I know they use websockets a lot.

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.

Should the PR title say websocket: instead of webtransport:?

p2p/transport/websocket/addrs.go Outdated Show resolved Hide resolved
localAddrChan := make(chan addrWrapper, 1)

transport := &http.Transport{
DialContext: func(ctx context.Context, network, addr string) (net.Conn, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we setting the DialContext here and below (in the isWss branch)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

To fill the localAddr into localAddrChan in case !isWss. This is the hackiest part of this. It doesn't seem like the websocket library lets us see the localAddr for the connection (which we need to fulfill the net.Conn interface). We can get it as part of the underlying dial, which is what I'm doing here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we a !isWss code path and set it there? That would make the code slightly easier to understand.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

p2p/transport/websocket/conn.go Show resolved Hide resolved
@MarcoPolo MarcoPolo changed the title webtransport: Replace gorilla websocket transport with nhooyr websocket transport websocket: Replace gorilla websocket transport with nhooyr websocket transport Jan 5, 2023
@MarcoPolo
Copy link
Collaborator Author

MarcoPolo commented Jan 5, 2023

@MarcoPolo
Copy link
Collaborator Author

I'm going to test this with the interop tester as well :)

@Jorropo
Copy link
Contributor

Jorropo commented Jan 6, 2023

Sorry for the dumb question but why ?

I know that gorilla websocket is no longer maintained, it's not like the websocket protocol is evolving and it probably wont in the near to medium future. Did we found issues in gorilla websocket that is forcing us to change ?

https://pkg.go.dev/nhooyr.io/websocket?tab=importedby has far less imports than https://pkg.go.dev/github.com/gorilla/websocket?tab=importedby (~500 vs ~18k), just by the shear number of users gorilla websocket and the "background fuzzing" this yielded I think most of it's bug to have been fixed already.

Also, I don't know what is the status on nhooyr websocket support, but 7 commits in 2 years doesn't seems very active to me, maybe nhooyr websocket is feature complete (again websocket isn't a protocol that has seen any recent activity) justifying such low activity idk.

I just would like to see more rational, the very remote view I see is that we are replacing a well tested explicitly abandoned project that had recent activity, with a less tested sparsely recently active one.


I think we should wait until we find an issue in gorilla websocket to try to pick an other lib, if it's working why are we trying to change it ?
This also leaves more time for someone else to pick up the project, this is an extremely popular project, I would be very surprised if in a year we don't see maintained forks of it.

@marten-seemann
Copy link
Contributor

I know that gorilla websocket is no longer maintained, it's not like the websocket protocol is evolving and it probably wont in the near to medium future. Did we found issues in gorilla websocket that is forcing us to change ?

It's not a good idea to depend on unmaintained code. We're not having any issues with Gorilla right now, but these are the kind of things that come back and bite you 6 months in the future, if you don't take care of them.

https://pkg.go.dev/nhooyr.io/websocket?tab=importedby has far less imports than https://pkg.go.dev/github.com/gorilla/websocket?tab=importedby (~500 vs ~18k), just by the shear number of users gorilla websocket and the "background fuzzing" this yielded I think most of it's bug to have been fixed already.

I can see this as an argument when comparing a library with 5 vs 180 imports, but 500 seems plenty.

This also leaves more time for someone else to pick up the project, this is an extremely popular project, I would be very surprised if in a year we don't see maintained forks of it.

Gorilla had been searching for a new maintainer since 2018 (!!), and nobody stepped up to take the job. I'm not very confident that somebody will do so now that it's abandoned. There'll certainly be people forking it, and making minor changes, but that's a very different thing than actually maintaining a project.

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 is now getting interop-tested using our handshake test, right?

p2p/transport/websocket/conn.go Show resolved Hide resolved
@marten-seemann marten-seemann merged commit 27cce4f into master Feb 7, 2023
marten-seemann added a commit that referenced this pull request May 9, 2023
marten-seemann added a commit that referenced this pull request May 9, 2023
marten-seemann added a commit that referenced this pull request May 11, 2023
* Revert "websocket: don't set a WSS multiaddr for accepted unencrypted conns (#2199)"

This reverts commit eeb685f.

* Revert "websocket: Don't limit message sizes in the websocket reader (#2193)"

This reverts commit 2fe6600.

* Revert "websocket: Replace gorilla websocket transport with nhooyr websocket transport (#1982)"

* websocket: don't set a WSS multiaddr for accepted unencrypted conns
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.

Migrate off github.com/gorilla/websocket
4 participants