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

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
Assignees
Milestone

Comments

@mattn
Copy link
Member

@mattn 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

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Nov 18, 2015

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

This comment has been minimized.

Copy link
Member Author

@mattn 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

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Nov 18, 2015

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

This comment has been minimized.

Copy link
Member Author

@mattn 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

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Nov 18, 2015

But not what?

@mattn

This comment has been minimized.

Copy link
Member Author

@mattn mattn commented Nov 18, 2015

Response always compressed even though I set DisableCompression=true.

@mattn

This comment has been minimized.

Copy link
Member Author

@mattn mattn commented Nov 18, 2015

Additional Information.

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

Twitter still reply gzip response.

@karaziox

This comment has been minimized.

Copy link

@karaziox karaziox commented Nov 21, 2015

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

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Nov 21, 2015

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

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Nov 26, 2015

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

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Nov 26, 2015

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

bradfitz added a commit that referenced this issue Nov 26, 2015
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
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.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.