Add support for fasthttp

[leavengood]

So this is a bit of a "speculative" PR in that I don't know if you guys are open to this kind of contribution and also whether the way I have written it matches the rest of the project.

The basic idea is I need websocket support in a project which uses the relatively new Go HTTP package fasthttp.

Gorilla websocket seemed like the best option to add this.

As you can see in the diff I have refactored various non-exported utility functions to make them usable from both net/http and fasthttp code, and then I added a FastHTTPUpgrader. The way fasthttp does hijacking is quite a bit different (and better IMO) than net/http, so the FastHTTPUpgrader .UpgradeHandler() is quite a bit more sane than the one for the standard Upgrader.Upgrade().

The existing tests pass but I did not add any tests for my addition. I could do that but didn't want to spend the time if this PR would be rejected.

[leavengood]

OK, Travis failed because fasthttp requires Go 1.4+. If that is an issue for this project then I may just need to keep this in my copy.

[leavengood]

I pulled the fasthttp stuff into its own file and added a // +build go1.4 comment.

[leavengood]

I still need to update some of the copyright years and add the copyright header to the new file. I could do a new commit or --amend this one, depending on what you prefer (and if you can accept this PR.)

[garyburd]

This is the first I've heard of the fasthttp package. I'll need to learn more about the package before deciding if I want add a dependency to the package. If a dependency is added, it will need to be in a new package (github.com/gorilla/websocket/need-a-good-name-here).

The API in this PR is inconvenient for the many applications that need to access the request before upgrading the connection. The API does not provide a way for applications to set cookies on the handshake response. I suggest changing the API to:

type FastHTTPUpgrader struct {
    ReadBufferSize, WriteBufferSize int
    Subprotocols []string
    CheckOrigin func(ctx *fasthttp.RequestCtx) bool
}

// Upgrade upgrades the HTTP server connection to the WebSocket protocol and calls f with 
// the websocket connection. The responseHeader is included in the response to the client's
// upgrade request. Use the responseHeader to specify cookies (Set-Cookie) and the 
// application negotiated subprotocol (Sec-Websocket-Protocol).
func (f *FastHTTPUpgrader) Upgrade(ctx *fasthttp.RequestCtx,  responseHeader http.Header, f func(*Conn)) error  {
}

I am not a fan of the constants added in the PR. I think the code origin := r.Header["Origin"] is easier to understand than origin := r.Header[originHeader] because I don't need to know what originHeader is to understand the line of code.

[leavengood]

Hi Gary,

I think fasthttp is a pretty cool package. I know net/http is a bit of a "standard" in the Go community and I would have never thought of replacing it, but fasthttp has some interesting ideas, and it is certainly very fast. It is very new but became very useful for myself and a colleague on a work project where net/http made our job much, much harder and fasthttp made it much, much easier (mainly because it has an API based on byte slices.) Here is a benchmark from the author where it is faster than nginx on serving files (I haven't tried this myself to verify though):

https://github.com/valyala/fasthttp/tree/master/examples/fileserver

As far as your recommended changes, it is still possible to add cookies with the current design because of how fasthttp works:

upgrader := &websocket.FastHTTPUpgrader{
    CheckOrigin: func(ctx *fasthttp.RequestCtx) bool { return true },
    Handler:     someWebsocketHandler,
}
addCookies := func(ctx *fasthttp.RequestCtx) {
    cookie := &fasthttp.Cookie{}
    cookie.SetKey("MyKey")
    cookie.SetValue("MyValue")
    ctx.Response.Header.SetCookie(cookie)
    upgrader.UpgradeHandler(ctx)
}
if err := fasthttp.ListenAndServe(":12345", addCookies); err != nil {
    log.Fatalf("Error in ListenAndServe: %s", err)
}

Result:

Though I do like the idea of having the callback as a parameter to avoid the annoying issue if it not being provided in the struct. But the reason I didn't do that was to make UpgradeHandler a fasthttp.RequestHandler (which is that packages version of ServeHTTP.) But obviously you chose to not use ServeHTTP in Upgrader so it probably would not be a huge deal for fasthttp too. Though in your case you had no choice because of the ServeHTTP interface and how net/http hijacking works. Now that you see an alternative maybe you can see the benefits of fasthttp 😄

As for the constants, we can probably agree to disagree. I see your point but then the same string is repeated in multiple places because I only added the constants when the string was used repeatedly. But I can change that if you really dislike it.

[leavengood]

One major issue with putting this in a sub-package of gorilla/websocket is then it couldn't use any of the unexported utility functions. Most especially we would need to export some way to make a websocket.Conn.

If you really don't want to make fasthttp a dependency (which I do understand), all I really need is the above exported function. Maybe something like:

func NewServerConn(conn net.Conn, subprotocol string, readBufferSize, writeBufferSize int) *Conn

Which of course just calls newConn.

There would be some repeated code for the handshake stuff, but nothing too crazy. Mainly I don't see a reason to reinvent websocket.Conn since it is independent of any HTTP package.

Then I could put all this into a new fastwebsocket or fasthttpws package or whatever, or it could be added to fasthttp itself.

[garyburd]

The Gorilla upgrade API is designed the way it is because it's convenient for applications. The design was not dictated by design of the net/http hijack API.

A sub-package can be supported by moving utility functions to an internal package.

The proposed NewServerConn requires more options. The handshake response headers and compression options are the two that come to mind. I expect that more options will be added in the future.

[leavengood]

The Gorilla upgrade API is designed the way it is because it's convenient for applications. The design was not dictated by design of the net/http hijack API.

I like this package and the design but a big reason you need the responseHeader parameter in Upgrade() is because of how the net/http API works. You can't really set headers and then pass along the ResponseWriter, most especially if you are hijacking. The fasthttp API allows for setting headers in the response inside the RequestCtx and then passing it along for websocket upgrading. That is just the "style" used for fasthttp, as I showed above in the cookies example.

As for the subpackage yeah I think an internal package could work, it would just require Go 1.4 or above I think.

Either way: what do you think the next steps should be on this? I'm working today and then will take a long vacation for the holidays. But if I get a chance I might be able to do some work on this. Obviously no one else really needs it at this point so there isn't a huge hurry.

[garyburd]

Godoc.org lists two importers of the package. There are two contributors. The package does not have enough traction for this project to take on code specific to fasthttp.

I suggest that fasthttp provide an adaptor from its API to the net/http API.

An application can set headers on a response writer header map and pass the response writer along. An application can set values on the header map and later hijack the connection.

The internal package structure can be used on all versions of Go. Go 1.4 and above is required restrict access to the packages per the rules in the design document.

[leavengood]

I'm surprised there are even that many importers this early on. I understand your reasoning in not wanting to take on code for such a new project.

I think making an adaptor between net/http and fasthttp would be pretty tricky, especially for hijacking. My understanding is that the fasthttp author broke away from the net/http API because it limited certain optimization possibilities.

I'll just close this PR and maintain my fork with the fasthttp websocket upgrader. Later on if fasthttp gets more traction we can reconsider merging.

[amitu]

@garyburd, the situation has changed since then. Can we reconsider it now?

[garyburd]

How has the situation changed? The issues above have not been addressed.

[leavengood]

fasthttp has gotten quite a bit more popular and I've seen a good amount of interest in my fork, though I have not had as much time to maintain it, as my work project changed. Life would be easier for everyone if you guys could merge my code.

[nikunjy]

I use fasthttp too. It is a pretty neat package. What is the main problem in merging this PR ?
Since fasthttp is a pretty popular package just like this project. What do you think @garyburd ?

Although there is an adaptor for fasthttp here
https://github.com/valyala/fasthttp/tree/master/fasthttpadaptor

[elithrar]

Adding fasthttp support to gorilla/websockets:

• Adds more code: even if the fasthttp hijacking logic is "simpler" (which I've not vetted myself), adding more code adds to the maintenance.
• Has almost no real world benefit, given that the Upgrade request/response is a minimal part of the WebSocket lifetime and does not improve the performance of the WebSocket itself.
• Requires library API changes: at the least, exposing internal methods and adding more public API surface area.

Please correct me if otherwise.

[nikunjy]

@elithrar You are correct on all counts except one in my opinion. There doesn't need to be a dependency between this package and net/http. So it seems to me that making certain methods public would not be such a bad idea ?
I stumbled on this issue while looking to use websockets, I am not super passionate about the PR either cause I can achieve this on my own too. Although I DO think certain methods might be useful to expose in the public API.

[elithrar]

This package should have a dependency with net/http so that package users don't have to stitch a bunch of logic together and hope for the best. Reviewing many of the issues created here, making it more "manual" to get started would turn many away, for minimal benefit. If there were no defacto Go HTTP library, sure, but that's not where we are today.

…

On Sun, May 21, 2017 at 1:34 PM Nikunj Yadav ***@***.***> wrote: @elithrar <https://github.com/elithrar> You are correct on all counts except one in my opinion. There doesn't need to be a dependency between this package and net/http. So it seems to me that making certain methods public would not be such a bad idea ? I stumbled on this issue while looking to use websockets, I am not super passionate about the PR either cause I can achieve this on my own too. Although I DO think certain methods might be useful to expose in the public API. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#98 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABIcMzals_iKUuNxpf0J7rgZ2__Sc35ks5r8J_UgaJpZM4G14xd> .

[garyburd]

Here's a summary of the issues with this PR:

• It is not acceptable to add a dependency on fasthttp from the github.com/gorilla/websocket package.
• The API makes it necessarily difficult to access the request before upgrading the connection or to set cookies in the handshake response.
• PR makes the code more difficult to understand by replacing strings like "Origin" with a constant.

This is based on a quick skim of my comments above. There may be other problems.

[garyburd]

Open a new issue to discuss fasthttp support. Please limit discussion here to this specific PR.

[leavengood]

As I have mentioned before I don't really have time to maintain these changes I made. At the moment I don't have need of them.

As @garyburd is suggesting there is probably a cleaner way of adding support for fasthttp to gorilla. Both projects have probably changed a lot since I wrote this.

Either way it really wasn't rocket science to implement the upgrade logic so I would suggest someone take a look at what I did, consider Gary's advice, and do a new PR which adds support for fasthttp in a way which this project can merge.

That might involve making some currently private APIs public, which I'm not sure gorilla would want to do, but at this point I leave that up to you guys, because bottom line I won't be working on this any time soon.