websocket: Replace gorilla websocket transport with nhooyr websocket transport #1982
Conversation
| localAddrChan := make(chan addrWrapper, 1) | ||
|
|
||
| transport := &http.Transport{ | ||
| DialContext: func(ctx context.Context, network, addr string) (net.Conn, error) { |
There was a problem hiding this comment.
Why are we setting the DialContext here and below (in the isWss branch)?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Can we a !isWss code path and set it there? That would make the code slightly easier to understand.
There was a problem hiding this comment.
It's harder to understand imo: https://gist.github.com/MarcoPolo/ddb3c159d1358d6e261a78282425839c
MarcoPolo
changed the title
|
I'm going to test this with the interop tester as well :) |
|
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 ? |
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.
I can see this as an argument when comparing a library with 5 vs 180 imports, but 500 seems plenty.
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. |
…bsocket transport (#1982)"
…bsocket transport (#1982)"
* 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
fixes #1970