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

Jump to bottom

x/net/http2: Transport should do transparent gzip like HTTP/1 #13298

Closed
mattn opened this issue Nov 18, 2015 · 11 comments
Closed

x/net/http2: Transport should do transparent gzip like HTTP/1 #13298

mattn opened this issue Nov 18, 2015 · 11 comments
Labels
FrozenDueToAge
Milestone
Go1.6

Comments

@mattn
Copy link
Member

mattn commented Nov 18, 2015 

package main

import (
    "bytes"
    "compress/gzip"
    b64 "encoding/base64"
    "encoding/json"
    "fmt"
    "io/ioutil"
    "log"
    "net/http"
    "net/url"
    "strconv"
)

const (
    ConsumerKey    = "XXXXXXXXXXXXXXXXXXXXXX"
    ConsumerSecret = "XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX"
)

func main() {
    client := http.DefaultClient
    encodedKeySecret := b64.StdEncoding.EncodeToString([]byte(fmt.Sprintf("%s:%s",
        url.QueryEscape(ConsumerKey),
        url.QueryEscape(ConsumerSecret))))
    reqBody := bytes.NewBuffer([]byte(`grant_type=client_credentials`))
    req, err := http.NewRequest("POST", "https://api.twitter.com/oauth2/token", reqBody)
    if err != nil {
        log.Fatal(err, client, req)
    }
    req.Header.Add("Authorization", fmt.Sprintf("Basic %s", encodedKeySecret))
    req.Header.Add("Content-Type", "application/x-www-form-urlencoded;charset=UTF-8")
    req.Header.Add("Content-Length", strconv.Itoa(reqBody.Len()))
    resp, err := client.Do(req)
    if err != nil {
        log.Fatal(err, resp)
    }
    defer resp.Body.Close()

    var respBody []byte
    // WHY THIS IS REQUIRED?
    if resp.Header.Get("Content-Encoding") == "gzip" {
        gr, err := gzip.NewReader(resp.Body)
        if err != nil {
            log.Fatal(err)
        }
        respBody, err = ioutil.ReadAll(gr)
    } else {
        respBody, err = ioutil.ReadAll(resp.Body)
    }
    if err != nil {
        log.Fatal(err, respBody)
    }

    var b struct {
        AccessToken string `json:"access_token"`
    }
    json.Unmarshal(respBody, &b)
    twitterEndPoint := "https://api.twitter.com/1.1/application/rate_limit_status.json"
    req, err = http.NewRequest("GET", twitterEndPoint, nil)
    if err != nil {
        log.Fatal(err)
    }
    req.Header.Add("Authorization",
        fmt.Sprintf("Bearer %s", b.AccessToken))
    resp, err = client.Do(req)
    if err != nil {
        log.Fatal(err, resp)
    }
    defer resp.Body.Close()
    respBody, err = ioutil.ReadAll(resp.Body)
    if err != nil {
        log.Fatal(err)
    }

    fmt.Println("response Status:", resp.Status)
    fmt.Println("response Headers:", resp.Header)
    fmt.Println(string(respBody))
}

try with your consumer keys. twitter send http2 response in recently.
the response Body should be decoded because content-encoding is gzip. but not decoded Body.

/cc @bradfitz
@bradfitz
Copy link
Contributor

Twitter is replying with "Content-Encoding: gzip" even without "Accept-Encoding: gzip"?

The http2 transport is not sending "Accept-Encoding: gzip". Thus, it's not the Go Transport's job to decompress it.

The HTTP/1 Transport code only decompresses it when it asks for it itself, using the logic in transport.go:

        // Ask for a compressed version if the caller didn't set their                                                                                                                       
        // own value for Accept-Encoding. We only attempt to                                                                                                                                 
        // uncompress the gzip stream if we were the layer that                                                                                                                              
        // 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: http://www.gzip.org/zlib/zlib_faq.html#faq38                                                                                                                         
                //                                                                                                                                                                           
                // Note that we don't request this for HEAD requests,                                                                                                                        
                // due to a bug in nginx:                                                                                                                                                    
                //   http://trac.nginx.org/nginx/ticket/358                                                                                                                                  
                //   https://golang.org/issue/5522                                                                                                                                           
                //                                                                                                                                                                           
                // 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")
        }

So, yes, HTTP/2 should do the same. But it's a little weird that Twitter is replying like that too.

@bradfitz bradfitz changed the title http2 client conn doesn't decode gziped response x/net/http2: Transport should do transparent gzip like HTTP/1 Nov 18, 2015
@bradfitz bradfitz self-assigned this Nov 18, 2015
@bradfitz bradfitz added this to the Go1.6 milestone Nov 18, 2015
@mattn
Copy link
Member Author

mattn commented Nov 18, 2015

The http2 transport is not sending "Accept-Encoding: gzip". Thus, it's not the Go Transport's job to decompress it.

Yes, I looked the code you pointed before. And I wonder why HTTP/1 works fine but HTTP/2 not.

@bradfitz
Copy link
Contributor

It works fine because HTTP/1 is decompressing it because it asked for it.

HTTP/2 is not asking for it, so it should not have to decompress it. Twitter should not be sending uncompressed content without "Accept-Encoding: gzip" in the request.

Try HTTP/1 with Transport.DisableCompression set to true. I bet it would also fail, if it's a Twitter problem.

@mattn
Copy link
Member Author

mattn commented Nov 18, 2015

I bet it would also fail, if it's a Twitter problem.
Yeah, indeed.

http.DefaultTransport.(*http.Transport).DisableCompression = true

I put this on top of code but not. :(

@bradfitz
Copy link
Contributor

But not what?

@mattn
Copy link
Member Author

mattn commented Nov 18, 2015

Response always compressed even though I set DisableCompression=true.

@mattn
Copy link
Member Author

mattn commented Nov 18, 2015

Additional Information.

req.Header.Add("Accept-Encoding", "identity")

Twitter still reply gzip response.

@karaziox
Copy link

I just hit this problem when doing oauth2 authentication with facebook. It seems the facebook http2 implementation doesn't care for the Accept-Encoding header and sends gzip no matter what. Manually adding the Accept-Encoding: gzip to the request will correctly fix the problem.

I suppose this is because a previous draft of the spec required the support for gzip, they removed that since. Seems more of a bug on Twitter/Facebook side than on go. I'll let you judge.

@bradfitz
Copy link
Contributor

I (or somebody) will implement this before Go 1.6.

@gopherbot
Copy link

CL https://golang.org/cl/17241 mentions this issue.

@gopherbot
Copy link

CL https://golang.org/cl/17242 mentions this issue.

bradfitz added a commit that referenced this issue Nov 26, 2015
@bradfitz
net/http: add test that automatic gzip works for HTTP2's Transport
e91019d 
And updates h2_bundle.go with the fix from x/net/http2.

Fixes #13298
Updates #6891

Change-Id: Ia25f22fa10e2a64b9d59211269882681aa18c101
Reviewed-on: https://go-review.googlesource.com/17241
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: Andrew Gerrand <adg@golang.org>
@golang golang locked and limited conversation to collaborators Nov 27, 2016
c3mb0 pushed a commit to c3mb0/net that referenced this issue Apr 2, 2018
@bradfitz
http2: add automatic gzip compression for the Transport
43fca76 
Tests are in net/http (clientserver_test.go, TestH12_AutoGzip)
from https://golang.org/cl/17241

Fixes golang/go#13298

Change-Id: I3f0b237ffdf6d547d57f29383e1a78c4f272fc44
Reviewed-on: https://go-review.googlesource.com/17242
Reviewed-by: Andrew Gerrand <adg@golang.org>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
Go1.6
Development

No branches or pull requests
4 participants
@bradfitz @mattn @karaziox @gopherbot