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

Check Content-Length against minSize and passthrough writes if no gzip #71

Merged
merged 1 commit into from
Aug 14, 2018
Merged

Conversation

jameshartig
Copy link
Contributor

Currently there's no way to generate a HEAD response with the correct
headers as the GET unless you set the minSize as 0, but then gzip headers
will be written in Close. Instead, allow a Write(nil) that will set the
correct headers based on the Content-Length/Content-Type headers and only
initialize a writer if there is a non-zero-length Write. If the
Content-Length cannot be determined, you cannot generate the response
because it cannot know if minSize would've been met.

Fixes #70

Currently there's no way to generate a HEAD response with the correct
headers as the GET unless you set the minSize as 0, but then gzip headers
will be written in Close. Instead, allow a Write(nil) that will set the
correct headers based on the Content-Length/Content-Type headers and only
initialize a writer if there is a non-zero-length Write. If the
Content-Length cannot be determined, you cannot generate the response
because it cannot know if minSize would've been met.

Additionally, if we determined that the request should not be compressed
we should passthrough writes immediately rather than waiting until
Close.

Fixes #70
Fixes #64
@jameshartig
Copy link
Contributor Author

I also handled the issue mentioned in #64.

The benchmarks seem to show the BenchmarkGzipHandler_S2k-8 got faster while the BenchmarkGzipHandler_P20k-8 and BenchmarkGzipHandler_P100k-8 got slower.

Before:
BenchmarkGzipHandler_S2k-8     	     200	   8180495 ns/op
BenchmarkGzipHandler_S20k-8    	     100	  24256560 ns/op
BenchmarkGzipHandler_S100k-8   	      20	 100509175 ns/op
BenchmarkGzipHandler_P2k-8     	    1000	   1766909 ns/op
BenchmarkGzipHandler_P20k-8    	     200	   5878452 ns/op
BenchmarkGzipHandler_P100k-8   	      50	  26159506 ns/op

After:
BenchmarkGzipHandler_S2k-8     	     200	   6932798 ns/op
BenchmarkGzipHandler_S20k-8    	     100	  24246411 ns/op
BenchmarkGzipHandler_S100k-8   	      20	 101508020 ns/op
BenchmarkGzipHandler_P2k-8     	    1000	   1691251 ns/op
BenchmarkGzipHandler_P20k-8    	     200	   7326576 ns/op
BenchmarkGzipHandler_P100k-8   	      30	  33444100 ns/op

@jameshartig jameshartig changed the title Support for HEAD responses and Content-Length checking against minSize Check Content-Length against minSize and passthrough writes if no gzip Aug 8, 2018
@jameshartig
Copy link
Contributor Author

@jprobinson Not sure if I should @ you or if you get notifications from PRs. Thanks!

@jprobinson
Copy link
Contributor

@fastest963 Hi! Sorry, I've been a bit busy. I've seen this but will need to find some time to review the PR. I'll try and get to it soon.

@jameshartig
Copy link
Contributor Author

@jprobinson okay, thanks! No problem, just wanted to check on it!

Copy link
Contributor

@jprobinson jprobinson left a comment

Choose a reason for hiding this comment

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

🤐 🌮 !

This looks pretty good! Sorry it took me so long to review it.

@jprobinson jprobinson merged commit c551b6c into nytimes:master Aug 14, 2018
@jprobinson
Copy link
Contributor

thanks again, @fastest963!

@jameshartig jameshartig deleted the head branch August 14, 2018 21:55
@jameshartig
Copy link
Contributor Author

No problem! Thanks for merging!

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.

2 participants