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

x/net/websocket: remedy neglect; merge with gorilla websocket? #18152

Open
zombiezen opened this issue Dec 1, 2016 · 55 comments

Comments

@zombiezen
Copy link
Contributor

commented Dec 1, 2016

It is not well maintained, and essentially everyone uses https://github.com/gorilla/websocket.

This was raised as a concern in #17244: because it is in the x/net repository, people attach greater value to it than it really ought to have.

/cc @dsnet

EDIT 2016-12-02: Copying in my better-phrased problem statement from later in the thread:

The problem I see is that the ownership/maintenance story of x/net/websocket is unclear. It may suit your needs, and that is fine! However, it's not getting the same level of support as the standard library or even gorilla/websocket; it has no established owner AFAIK.

Along the lines of #17244, there is concern that it elevates the status and visibility of the package. I've anecdotally heard of it confusing many new users whose needs were much better filled with gorilla/websocket. I have not heard of someone using gorilla/websocket and then deciding to use x/net/websocket instead.

@zombiezen zombiezen added the Proposal label Dec 1, 2016

@bradfitz

This comment has been minimized.

Copy link
Member

commented Dec 1, 2016

We can't just delete something and break people who aren't vendoring it.

We can document that it's deprecated perhaps.

Or somebody could maintain it.

@bradfitz bradfitz added this to the Proposal milestone Dec 1, 2016

@zombiezen

This comment has been minimized.

Copy link
Contributor Author

commented Dec 1, 2016

Deprecated might be fine, but my experience is that I (and other devs I've talked to) have wasted time trying to use the package only to find out it fails in production and that Gorilla's package works.

@dsnet

This comment has been minimized.

Copy link
Member

commented Dec 1, 2016

I support a strong deprecation message. Would that message endorse the Gorilla implementation?

If we implement golang/lint#238, I think that will help people avoid wasting time developing with deprecated packages or features.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Dec 1, 2016

"fails in production" -- what are the open bugs? There's not much to WebSockets. I'm afraid we're going to spend more time discussing things on this bug than we would just fixing any such issues.

@cespare

This comment has been minimized.

Copy link
Contributor

commented Dec 1, 2016

Beyond any bugs that could be fixed by maintenance, the deeper problem with x/net/websocket is that it has the wrong API. It puts an io.Reader/Writer interface on top of what is fundamentally a message-based protocol.

I agree it should be documented as deprecated in favor of gorilla/websocket.

@zombiezen

This comment has been minimized.

Copy link
Contributor Author

commented Dec 1, 2016

It's been long enough since I last tried to use it, but off-hand, you cannot implement a correct websocket reverse proxy with the current API, since it tries to implement io.Reader/io.Writer. Websocket, for better or for worse, is a frame-oriented protocol, not a stream-oriented protocol.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Dec 1, 2016

I agree it has the wrong API. But we could add ReadMessage and WriteMessage to it and deprecate Read and Write.

I am in favor of maintaining less code, though, (not that we ever maintained x/net/websocket much?), and I have no personal or sentimental connection to x/net/websocket, and we actually use gorilla/websocket in Camlistore these days, but it still feels core enough that we should have an official answer for websockets in x/net.

I suppose this all kinda depends on the policy for x/* we figure out in #17244.

@davecheney

This comment has been minimized.

Copy link
Contributor

commented Dec 1, 2016

@cznic

This comment has been minimized.

Copy link
Contributor

commented Dec 1, 2016

Please do not deprecate it. At work, we are using golang.org/x/net/websocket exclusively and we have never had any problem with it*. Quite the opposite, in our use case it's more robust than what we've ever expected.

*: Of course, that says nothing whatsoever about any trouble anyone else might have with it.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Dec 1, 2016

@cznic, "deprecate" does not mean "delete".

@cznic

This comment has been minimized.

Copy link
Contributor

commented Dec 1, 2016

Yep. But I guess it means "no more bug fixes for you, ever".

@bradfitz

This comment has been minimized.

Copy link
Member

commented Dec 1, 2016

No, not really. Deprecated is not black & white. We continue to fix bugs in other deprecated stuff when there's a good bug report and a clear fix and super minimal risk of breaking existing users. It just means we're not adding features to it, or means that a better way is known to exist.

@cznic

This comment has been minimized.

Copy link
Contributor

commented Dec 1, 2016

Great news, honestly. But then again, why to mark it deprecated in the first place? We can cope with it getting no attention from people not loving it, but we do and if we run into a bug we need to resolve, we would be more than happy to contribute the fix.

Status quo anybody?

@bradfitz

This comment has been minimized.

Copy link
Member

commented Dec 1, 2016

But then again, why to mark it deprecated in the first place?

To tell users they might be happier elsewhere.

@zombiezen

This comment has been minimized.

Copy link
Contributor Author

commented Dec 2, 2016

Precisely to @bradfitz's point: Deleting may be too strong, but I want to send a strong signal that there's a much better maintenance story for gorilla/websocket. Effort spent on x/net/websocket may be better spent on gorilla/websocket.

@gopherbot

This comment has been minimized.

Copy link

commented Dec 2, 2016

CL https://golang.org/cl/33806 mentions this issue.

gopherbot pushed a commit to golang/net that referenced this issue Dec 2, 2016
websocket: mention the gorilla package
Updates golang/go#18152

Change-Id: Ia3df3f9668847690e60a2af0680cf1bd66042384
Reviewed-on: https://go-review.googlesource.com/33806
Reviewed-by: Ross Light <light@google.com>
@cznic

This comment has been minimized.

Copy link
Contributor

commented Dec 2, 2016

To tell users they might be happier elsewhere.

I really really really don't want to sound snarky, but by that logic, the Go repository might be marked deprecated as well - just because users might be happier with Rust, C, Java, ...?

Effort spent on x/net/websocket may be better spent on gorilla/websocket.

I have no intention, nor desire to suggest or propose, regardless how impossible and foolish that would be, to delete github.com/gorilla/websocket, just because we don't and probably never would use it.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Dec 2, 2016

I really really really don't want to sound snarky,

Yet you did. And it's not very productive. It's actually just noise in this thread.

If you want to be productive, though, step up and maintain golang.org/x/net/websocket. That's one of the options I listed in my first comment (#18152 (comment))

@zombiezen

This comment has been minimized.

Copy link
Contributor Author

commented Dec 2, 2016

I apologize, @cznic. The current proposal process biases bug titles to be action-based (do X) versus focused on problem statement, so I should have described the problem instead of a proposed plan of action. Maybe this is something we should consider improving in the proposal process.

The problem I see is that the ownership/maintenance story of x/net/websocket is unclear. It may suit your needs, and that is fine! However, it's not getting the same level of support as the standard library or even gorilla/websocket; it has no established owner AFAIK.

The other issue I see is along the lines of #17244, where there is concern that it elevates the status and visibility of the package. I've anecdotally heard of it confusing many new users whose needs were much better filled with gorilla/websocket. I have not heard of someone using gorilla/websocket and then deciding to use x/net/websocket instead.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Dec 2, 2016

And to be clear, @cznic, I'm quite serious about you maintaining x/net/websocket. As a user of it, you're better suited to be a maintainer of it than a random contributor looking for work to do. And you maintain enough other stuff that you know what the process entails.

@cznic

This comment has been minimized.

Copy link
Contributor

commented Dec 2, 2016

It's actually just noise in this thread.

I respectfully disagree. I see no proof why it's different (wrt people happier with something else). I'm ready to acknowledge you're right if you can actually explain your judgement in some other way than simply claiming my point has no point (just noise in your words).

Nonetheless...

If you want to be productive, though, step up and maintain golang.org/x/net/websocket.

And to be clear, @cznic, I'm quite serious about you maintaining x/net/websocket. As a user of it, you're better suited to be a maintainer of it than a random contributor looking for work to do. And you maintain enough other stuff that you know what the process entails.

Regardless of us (at work) never having a problem with it, how do you expect me to maintain x/net/websocket? Serious question from a person having only R/O access to it.

I apologize, @cznic. ...

@zombiezen Relly appreciated, but there's nothing to apologize for. I don't agree with the proposal, but that's nothing personal. If I made an impression otherwise then it's my duty to apologize for my fault - sorry.


PS: @ALL Getting the company's approval to use Go and/or code from golang.org/whatever is one thing. The same for code from other domains is not the same.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Dec 2, 2016

Regardless of us (at work) never having a problem with it, how do you expect me to maintain x/net/websocket? Serious question from a person having only R/O access to it.

If you send us CLs, we'll +2 and you can submit.
If others send you CLs, we'll give you access to +2 their CLs and you can submit.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Dec 2, 2016

I respectfully disagree. I see no proof why it's different (wrt people happier with something else). I'm ready to acknowledge you're right if you can actually explain your judgement in some other way than simply claiming my point has no point (just noise in your words).

I find your comparison between recommending a maintained package and recommending a whole alternate language not even remotely equivalent. They're pretty absurdly different. You also found it ridiculous, which is why you prefaced it with "I really really really don't want to sound snarky". You can't just say something outlandish and preface it with "Not to be $X or anything, but ....".

By the time most people are selecting a websocket package, they've already selected their programming language (Go, in this case). On the off chance they haven't yet selected their programming language (say, they're writing a prototype application to decide whether they like the Go ecosystem), that argues even more in favor of @zombiezen's point that we should point them to the solution that's going to be make them happy, rather than the first thing they stumbled into.

Hence https://golang.org/cl/33806

@groob

This comment has been minimized.

Copy link
Contributor

commented Dec 2, 2016

Thank you Brad,

Not to add noise to the issue, but we've recently had to introduce websockets to our app and had this debate do research to find out wether x/net/websocket would work. Luckily I was aware of discussion in #17244 which mentions websocket vs the gorilla package. The mention will help others save time and avoid using something just because it looks official.

@cznic

This comment has been minimized.

Copy link
Contributor

commented Dec 2, 2016

If others send you CLs, we'll give you access to +2 their CLs and you can submit.

Let's roll.

@leaxoy

This comment has been minimized.

Copy link

commented Dec 2, 2016

But how about implement it in standard package net/websocket?

@bradfitz

This comment has been minimized.

Copy link
Member

commented Dec 2, 2016

@leaxoy, no thanks. Unmaintained code in x/ is bad enough. We don't also want more code in std, especially if it's unmaintained. See https://golang.org/doc/faq#x_in_std

@garyburd

This comment has been minimized.

Copy link
Contributor

commented Dec 6, 2016

Is this the issue where we should discuss this, or should I open a new issue?

@bradfitz

This comment has been minimized.

Copy link
Member

commented Dec 6, 2016

@garyburd, let's repurpose this bug.

@bradfitz bradfitz changed the title proposal: do something about the neglected x/net/websocket x/net/websocket: remedy neglect; merge with gorilla websocket? Dec 6, 2016

@bradfitz bradfitz removed the Proposal label Dec 6, 2016

@bradfitz bradfitz modified the milestones: Unreleased, Proposal Dec 6, 2016

@garyburd

This comment has been minimized.

Copy link
Contributor

commented Dec 6, 2016

I'll follow up on email regarding licensing/CLA.

What would you omit from Gorilla package?

The move to a new import path provides an opportunity to improve the Gorilla API. The most important change is to restructure the API to allow for concurrent writers. I'd also remove the JSON methods from the connection type.

I was expecting that you would take a gorilla/websocket as a new package (x/net/websocket2 ?) or as a new types in x/net/websocket. Either way, the existing x/net/websocket would be rewritten as a wrapper around the gorilla code. The wrapper will make a best effort to be compatible the current x/net/websocket code given that x/net/websocket API exposes frames in the API while the Gorilla API exposes messages.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Dec 7, 2016

What would you omit from Gorilla package?

Could you send a dummy CL (to say, x/net/review/websocket) with just the API surface and all the implementation removed, and I can comment in Gerrit?

@jech

This comment has been minimized.

Copy link

commented Dec 7, 2016

there's a few things I'd omit or change in gorilla before adding it to x/net.

Is this the right place to discuss this? If it is, then I'd like to mention that we're using Websockets for sending frequent tiny updates (on the order of a few octets), and that a cursory reading through the Gorilla implementation doesn't show a way to send a small data frame without consing up a writer.

@dsnet

This comment has been minimized.

Copy link
Member

commented Dec 7, 2016

@jech, that sounds like an implementation detail, but it important to keep in mind since the choice of API does have an impact on performance.

@garyburd

This comment has been minimized.

Copy link
Contributor

commented Dec 8, 2016

@jech See gorilla/websocket@0868951. The message writer is no longer allocated on heap for the WriteMessage fast path.

@Inuart

This comment has been minimized.

Copy link

commented Sep 28, 2017

any updates?

@garyburd

This comment has been minimized.

Copy link
Contributor

commented Sep 28, 2017

There's agreement about moving the package, but I don't have time to do the work.

@pciet

This comment has been minimized.

Copy link
Contributor

commented Dec 25, 2017

Google Cloud Platform has a beta websocket API: https://issuetracker.google.com/issues/35886348

Despite lack of maintainers I'd still like to reopen the idea of a standard library package net/websocket for Go 2.

@ivoras

This comment has been minimized.

Copy link

commented Feb 14, 2018

I think I may be encountering this error when using Gorilla's websockets, which apparently isn't present in this implementation: gorilla/websocket#43 .

I have a WebSocket server, with two goroutines per connection: one reading messages and pushing them to a channel, and the other pulling them from the channel, processing them and writing the results to the connection. So far, it looks like at semi-random points I get spurious "unexpected EOF" messages on the Gorilla implementation. I yet need to try Google's.

@binary132

This comment has been minimized.

Copy link

commented Apr 30, 2018

Looks like Gorilla's ws implementation needs a maintainer.

gorilla/websocket#370

@nhooyr

This comment has been minimized.

Copy link
Contributor

commented Sep 22, 2018

@garyburd regarding your comment above

The most important change is to restructure the API to allow for concurrent writers.

Why is that important and what would this sort of API look like?

@nhooyr

This comment has been minimized.

Copy link
Contributor

commented Mar 30, 2019

I started working on a new WebSocket library for Go.

The main features being a minimal easy to use API that will work with WebSockets over HTTP/2.

https://github.com/nhooyr/websocket#websocket

Right now the entire implementation is stubbed out as I wanted to get some feedback on the API.

@jech

This comment has been minimized.

Copy link

commented Mar 30, 2019

@nhooyr

This comment has been minimized.

Copy link
Contributor

commented Mar 30, 2019

@jech

This comment has been minimized.

Copy link

commented Mar 30, 2019

@nhooyr

This comment has been minimized.

Copy link
Contributor

commented Apr 7, 2019

I just tagged v0.1.0, it passes all of the autobahn tests except for compression which I'll add later.

https://github.com/nhooyr/websocket

Let me know what you think.

edit: Have tagged v1.0.0 now 🎊

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.