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

proposal: net/http: accept "x-gzip" content encoding value #40765

Closed
melardev opened this issue Aug 13, 2020 · 9 comments
Closed

proposal: net/http: accept "x-gzip" content encoding value #40765

melardev opened this issue Aug 13, 2020 · 9 comments

Comments

@melardev
Copy link

Hi, would it be good if I submit the following diff file?
It basically treats x-gzip as gzip, as noted in the HTTP 0.9 RFC: https://www.ietf.org/rfc/rfc1945.txt section 3.5
For the request header, that may not be that important, but for the response it may save more than one response from legacy servers.

@@ -2012,7 +2012,8 @@ func (pc *persistConn) readLoop() {
 		}
 
 		resp.Body = body
-		if rc.addedGzip && strings.EqualFold(resp.Header.Get("Content-Encoding"), "gzip") {
+		if rc.addedGzip && (strings.EqualFold(resp.Header.Get("Content-Encoding"), "gzip") ||
+			strings.EqualFold(resp.Header.Get("Content-Encoding"), "x-gzip")) {
 			resp.Body = &gzipReader{body: body}
 			resp.Header.Del("Content-Encoding")
 			resp.Header.Del("Content-Length")
@@ -2368,9 +2369,9 @@ func (pc *persistConn) roundTrip(req *transportRequest) (resp *Response, err err
 	// requested it.
 	requestedGzip := false
 	if !pc.t.DisableCompression &&
-		req.Header.Get("Accept-Encoding") == "" &&
 		req.Header.Get("Range") == "" &&
 		req.Method != "HEAD" {
+
 		// Request gzip only, not deflate. Deflate is ambiguous and
 		// not as universally supported anyway.
 		// See: https://zlib.net/zlib_faq.html#faq39
@@ -2383,8 +2384,14 @@ func (pc *persistConn) roundTrip(req *transportRequest) (resp *Response, err err
 		// We don't request gzip if the request is for a range, since
 		// auto-decoding a portion of a gzipped document will just fail
 		// anyway. See https://golang.org/issue/8923
-		requestedGzip = true
-		req.extraHeaders().Set("Accept-Encoding", "gzip")
+		if req.Header.Get("Accept-Encoding") == "" || req.Header.Get("Accept-Encoding") == "gzip" {
+			requestedGzip = true
+			req.extraHeaders().Set("Accept-Encoding", "gzip")
+		} else if req.Header.Get("Accept-Encoding") == "x-gzip" {
+			requestedGzip = true
+			req.extraHeaders().Set("Accept-Encoding", "x-gzip")
+		}
+
 	}
 
 	var continueCh chan struct{}
@andybons andybons changed the title legacy gzip content coding support proposal: net/http: accept "x-gzip" content encoding value Aug 13, 2020
@gopherbot gopherbot added this to the Proposal milestone Aug 13, 2020
@andybons
Copy link
Member

@neild @bradfitz

@andybons
Copy link
Member

@melardev are you experiencing this in the wild? Which servers still serve x-gzip as their content-encoding?

@melardev
Copy link
Author

@andybons actually not, but I was trying to write some HTTP client code for HTTP 0.9 and reviewing the RFC this showed up, I asked myself if the remote server returns such response, would be golang able to decode it or not, looking at the code it seems there may be an issue if that code path is triggered, so I wanted to ask here if it would be interesting to add support for it and see what you think about that.

@melardev
Copy link
Author

Also, I don't know exactly when that function is being triggered, by default using HttpClient.Do() it is not, it would be interesting to know what would have happened if the client already adds the header "Accept-Encoding" to "gzip", the code as of now, it would not set the requestedGzip boolean flag to true, look at:
https://github.com/golang/go/blob/master/src/net/http/transport.go#L2504
It only sets the requestedGzip boolean to true if the header is absent and not when the header is present with gzip. But again, don't know exactly when that code is triggered.

@melardev
Copy link
Author

@andybons Digging a little bit further I detected a bug indeed, this same code is at h2_bundle.go which has a copy paste code (as the comment also state):
https://github.com/golang/go/blob/master/src/net/http/h2_bundle.go#L7500
Then the following check is bypassed because requestedGzip is false.
https://github.com/golang/go/blob/master/src/net/http/h2_bundle.go#L8474
So to reproduce the bug run the following:

client := http.Client{}
	request, err := http.NewRequest("GET", "https://httpbin.org/gzip", nil)
	request.Header.Set("Accept-Encoding", "gzip")

	if err != nil {
		log.Fatalln(err)
	}

	resp, err := client.Do(request)
	if err != nil {
		log.Fatalln(err)
	}

	content, _ := ioutil.ReadAll(resp.Body)
        // Garbage will be printed, gzip is not decoded!
	log.Println(string(content))

If we remove the line

request.Header.Set("Accept-Encoding", "gzip")

The code would work fine.
This would be my first contribution to such a big prorject, can I make a pull request to fix this bug?

@rsc
Copy link
Contributor

rsc commented Oct 21, 2020

We are not trying to support HTTP/0.9 - we removed support for it.
Nor are we trying to support hypothetical uses.
If there's no actual x-gzip use in the wild, let's not add the complexity here.
Do you know of any important need for x-gzip in real-world use cases?

@rsc rsc moved this from Incoming to Active in Proposals (old) Oct 21, 2020
@melardev
Copy link
Author

@rsc Hi, no I don't have a real-word use case, I just read that reviewing the HTTP 0.9 spec, I think we can close the issue then.

@rsc
Copy link
Contributor

rsc commented Oct 28, 2020

Based on the discussion above, this seems like a likely decline.

@rsc rsc moved this from Active to Likely Decline in Proposals (old) Oct 28, 2020
@rsc
Copy link
Contributor

rsc commented Nov 4, 2020

No change in consensus, so declined.

@rsc rsc closed this as completed Nov 4, 2020
@rsc rsc moved this from Likely Decline to Declined in Proposals (old) Nov 4, 2020
@golang golang locked and limited conversation to collaborators Nov 4, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
Development

No branches or pull requests

4 participants