Start plain response as soon as certain#74
Merged
jprobinson merged 2 commits intoAug 20, 2018
Conversation
When we get the response headers it might be pretty much already known whether we will have to serve plain content instead of gzipping. That is either when the content-length header is set with a small enough size, there is already an encoding in the response or the content-type is already set to a content we don't handle.
fsouza
approved these changes
Aug 20, 2018
Contributor
fsouza
left a comment
fsouza
left a comment
There was a problem hiding this comment.
@victorges thanks for fixing that.
re: introducing a state to the writer, I agree that it would make the logic easier to follow. Can you send the PR? :D
Contributor
Author
|
Great! I think I don't have access to merging this though, could you do it for me? I'll try to do the state thing some time in the next few days :) |
Contributor
|
Thanks, @victorges & @fsouza! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Using this library in one of my projects, I came to realise our API for Server-Sent Events stopped working when the request included an
Accept-Encoding: gzipheader.Investigating on the gziphandler code I found out that, even though I added only
application/jsonas the list of supported content types to be GZIPped, 2 little issues led to SSE stop working:handleContentTypefunction wasn't being checked until the response actually reached the specifiedminSize(which I left to the default). TheContent-Typewas already being set to an unsupported content type (text/event-stream) so we didn't need to wait all the first 1400 bytes before starting the plain response. Fixed this on the first commit.Flushfunc was not flushing the underlyingResponseWriterwhen we were not gzipping the response. This is needed as we make sureFlushis called after every event we write to the response. Fixed this on the second commit.One thing that is still slightly bothering me is that we are parsing the response headers every time the
Writefunction is called. I'm thinking of adding a little "state-enum" in thegzip.Writerstruct, indicating whether:I think this will make the logic much clearer as well. Please also let me know what you think of that idea and I could try to send it as a separate pull-request!