Skip to content

force option to attempt http2 #325

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

Closed
wants to merge 4 commits into from
Closed

force option to attempt http2 #325

wants to merge 4 commits into from

Conversation

golgoth31
Copy link

Resty does not use http2 golang conn. Forcing attempt option in transport allow resty to use http2

@codecov
Copy link

codecov bot commented Mar 9, 2020

Codecov Report

Merging #325 into master will decrease coverage by 0.15%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #325      +/-   ##
==========================================
- Coverage   95.64%   95.48%   -0.16%     
==========================================
  Files           9        9              
  Lines        1195     1197       +2     
==========================================
  Hits         1143     1143              
- Misses         30       31       +1     
- Partials       22       23       +1     
Impacted Files Coverage Δ
client.go 96.27% <100.00%> (-0.61%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ad527eb...9ead5bc. Read the comment docs.

Copy link
Member

@jeevatkm jeevatkm left a comment

Choose a reason for hiding this comment

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

@golgoth31 Thanks for submitting the PR.

By default Go HTTP client capable of handling HTTP2 and 1.1; what is the problem?

I would like to understand the problem you're trying to solve. How is a clone is solving the problem?

FYI, due to supported API
This PR has failing build https://www.travis-ci.org/github/go-resty/resty/jobs/671228244

@golgoth31
Copy link
Author

Hi, Thank you for your answer.
I'm ok to say that, by default, GO HTTP client is http/2 aware but, in the resty client definition, you create a totally new transport layer based on the Transport struct. This layer is not http/2 aware by default, leading your client not using http/2. It's not really clear but it's explained here: https://golang.org/src/net/http/transport.go

// ForceAttemptHTTP2 controls whether HTTP/2 is enabled when a non-zero
// Dial, DialTLS, or DialContext func or TLSClientConfig is provided.
// By default, use of any those fields conservatively disables HTTP/2.
// To use a custom dialer or TLS config and still attempt HTTP/2
// upgrades, set this to true.

I wrote a simple main file:

package main

import (
	"fmt"
	"net/http"
	"github.com/go-resty/resty/v2"
)

func main() {
	client := resty.New()
	rresp, _ := client.R().Get("https://httpbin.org/ip")
	fmt.Printf("resty response protocol: %s\n", rresp.RawResponse.Proto)

	httpclient := http.DefaultClient
	hresp, _ := httpclient.Get("https://httpbin.org/ip")
	fmt.Printf("net/http response protocol: %s\n", hresp.Proto)
}

The result is as follow:

  • without my patch:
    ❯ go run test.go
    resty response protocol: HTTP/1.1
    net/http response protocol: HTTP/2.0
  • with my patch:
    ❯ go run test.go
    resty response protocol: HTTP/2.0
    net/http response protocol: HTTP/2.0

As shown, the current resty client is not http/2.

For the CI problem, between 1.12 and 1.13, the http2 transport enabling has been changed. I will investigate on that.

@golgoth31 golgoth31 requested a review from jeevatkm April 9, 2020 09:03
@@ -24,6 +24,8 @@ import (
"strings"
"sync"
"time"

"golang.org/x/net/http2"
Copy link
Member

Choose a reason for hiding this comment

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

@golgoth31 Thanks for submitting PR. And providing insight into the issue.
golang.org/x/net/http2 package is part of the language since go1.6. So externally referring is not required.

I will investigate http2 and address it. Thanks again.

@jeevatkm
Copy link
Member

@golgoth31 Just now I have analyzed it using your code snippet.

  • It seems HTTP 2.0 broken go1.13 onwards
  • It was working until go1.12
  • So, it's clearly evident it was working up to go1.12 so I did not go further.

Results are -

Broken on go.14
Screen Shot 2020-05-10 at 10 10 45 PM

Broken on go.13
Screen Shot 2020-05-10 at 10 11 00 PM

Success on go.12
Screen Shot 2020-05-10 at 10 11 29 PM

Success on go.11
Screen Shot 2020-05-10 at 10 11 42 PM

@jeevatkm
Copy link
Member

@golgoth31 Closing this PR in-favor of #339

@jeevatkm jeevatkm closed this May 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants