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

Support gzip compression for websockets #300

Closed
systemfreund opened this issue May 24, 2017 · 11 comments
Closed

Support gzip compression for websockets #300

systemfreund opened this issue May 24, 2017 · 11 comments
Labels
Milestone

Comments

@systemfreund
Copy link
Contributor

A browser (at least chrome does it) usually sends an Accept-Encoding: gzip header with every request. For example:

GET /some-websocket-endpoint HTTP/1.1
Connection: Upgrade
Upgrade: websocket
Accept-Encoding: gzip, deflate, sdch, br

Now I am running fabio with gzip compression enabled and when i execute the example request fabio responds with a 500 error. It's not the upstream server sending the 500, but fabio. The error response is sent from here:

https://github.com/fabiolb/fabio/blob/master/proxy/http_raw_handler.go#L28

The reason this is failing is that w can't by type-cast (or how it's called in Go slang) to an http.Hijacker, as w is a GzipResponseWriter.

I am not sure if it's a proper fix, but what I've done to make it work was to modify the acceptsGzip function like this:

func acceptsGzip(r *http.Request) bool {
	return (strings.Contains(r.Header.Get(headerAcceptEncoding), encodingGzip) &&
		!strings.Contains(r.Header.Get("Upgrade"), "websocket"))
}
@magiconair
Copy link
Contributor

Is this on master? I think I've touched this code recently. I'll have a look later tonight.

@systemfreund
Copy link
Contributor Author

Yes I can reproduce this on master

@systemfreund
Copy link
Contributor Author

I was experimenting with master, because a similar issue regarding 500 errors and Websockets has been fixed recently and I was hoping that would solve my problem as it sounded very much like the same problem I am having. However, as you can see it was not the case.

@magiconair
Copy link
Contributor

I'll fix that tonight

@magiconair
Copy link
Contributor

I've pushed a fix that I think should fix this. Still need to write a test. Could you test this, please?

@magiconair magiconair added the bug label May 24, 2017
@systemfreund
Copy link
Contributor Author

systemfreund commented May 24, 2017

I appreciate your fast reaction! Unfortunately, the fix did not seem to help. For reference, I am running fabio with these parameters:

./fabio -proxy.gzip.contenttype='^(text/.*|application/(javascript|json|hal+json|xml)|.*\+(json|xml))(;.*)?$'

Then I am doing the problematic request:

nc localhost 9999 < /failing-request

GET /upstream HTTP/1.1
Host: localhost:9999
Connection: Upgrade
Upgrade: websocket
Accept-Encoding: gzip, deflate, sdch, br

I've added a fmt.Println() at the same location described above (http_raw_handler.go#28) and it indeed is running into the "not a hijacker" block again.

@systemfreund
Copy link
Contributor Author

systemfreund commented May 24, 2017

Here's how you can reproduce the problem:

  • run python -m http.server 1234
  • create file upstream.json:
    { "service": { "name": "upstream", "port": 1234, "tags": ["urlprefix-/upstream"], "check": { "id": "upstream", 
    "http": "http://localhost:1234", "interval": "10s", "timeout": "1s" } } }
    
  • run consul agent -dev -config-file=upstream.json
  • run
    fabio  -proxy.gzip.contenttype='^(text/.*|application/(javascript|json|hal+json|xml)|.*\+(json|xml))(;.*)$'
    
  • run nc localhost 9999 < failing-request (for contents of failing-request see previous comment)

@magiconair
Copy link
Contributor

It was worth a shot. I'll add a proper test and fix it. I've got a long weekend ahead so this will probably happen next week - just to set expectations.

magiconair added a commit that referenced this issue May 26, 2017
This patch fixes an error 500 when websocket clients are setting
an Accept-Encoding: gzip header.
@magiconair
Copy link
Contributor

Fixed it in the wrong place. The new patch should fix it.

Also, the websocket lib I'm using for testing doesn't make it easy to set headers. Therefore, writing a test requires a bit more work. I need to replace it anyway but I was able to reproduce the behavior with curl and the --compressed flag.

Could you have another look, please?

@systemfreund
Copy link
Contributor Author

I can confirm now that the fix is working. Thank you very much!

@magiconair
Copy link
Contributor

Cool. I'll write a test and merge it.

@magiconair magiconair changed the title HTTP 500 when a browser makes a websocket connection in conjunction with Accept-Encoding: gzip header Support gzip compression for websockets Jun 5, 2017
@magiconair magiconair added this to the 1.5.0 milestone Oct 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants