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

net/http: Proto when set is not used properly. #13985

Closed
harshavardhana opened this issue Jan 17, 2016 · 17 comments

Comments

Projects
None yet
4 participants
@harshavardhana
Copy link
Contributor

commented Jan 17, 2016

For example even after setting request Proto has no affect on how http.Client sends request, it always defaults to. 'HTTP/1.1'

$ cat http-test.go
package main

import (
    "fmt"
    "log"
    "net/http"
    "net/http/httputil"
)

func main() {
    req, err := http.NewRequest("GET", "https://www.google.com", nil)
    if err != nil {
        log.Fatalln(err)
    }
    req.Proto = "HTTP/1.0"
    client := &http.Client{}
    resp, err := client.Do(req)
    if err != nil {
        log.Fatalln(err)
    }
    reqTrace, err := httputil.DumpRequestOut(req, false)
    if err != nil {
        log.Fatalln(err)
    }
    respTrace, err := httputil.DumpResponse(resp, false)
    if err != nil {
        log.Fatalln(err)
    }
    fmt.Println(string(reqTrace))
    fmt.Println(string(respTrace))
}
$ go run http-test.go
GET / HTTP/1.1
Host: www.google.com
User-Agent: Go-http-client/1.1
Accept-Encoding: gzip


HTTP/1.1 200 OK
Transfer-Encoding: chunked
Alt-Svc: quic="www.google.com:443"; ma=600; v="30,29,28,27,26,25",quic=":443"; ma=600; v="30,29,28,27,26,25"
Alternate-Protocol: 443:quic,p=1
Cache-Control: private, max-age=0
Content-Type: text/html; charset=ISO-8859-1
Date: Sun, 17 Jan 2016 10:08:59 GMT
Expires: -1
P3p: CP="This is not a P3P policy! See https://www.google.com/support/accounts/answer/151657?hl=en for more info."
Server: gws
Set-Cookie: NID=75=OeIomPif8UPzAt2QI2qzIOeqiJo4jvQRmsKjBHWDpaXwDGizQB5lAxi0Lb3Dw_5sCeWvZmccGoZER2Pj3zuvY2PI982WCxDQBhU62CW1cZUmMqUNeT5vv5tDA1g_646320jRqqcKxFSuQCg; expires=Mon, 18-Jul-2016 10:08:59 GMT; path=/; domain=.google.com; HttpOnly
X-Frame-Options: SAMEORIGIN
X-Xss-Protection: 1; mode=block

Now testing with .Proto as 'HTTP/2.0' defaults to 'HTTP/1.1'

$ cat http-test.go
package main

import (
    "fmt"
    "log"
    "net/http"
    "net/http/httputil"
)

func main() {
    req, err := http.NewRequest("GET", "https://www.google.com", nil)
    if err != nil {
        log.Fatalln(err)
    }
    req.Proto = "HTTP/2.0"
    client := &http.Client{}
    resp, err := client.Do(req)
    if err != nil {
        log.Fatalln(err)
    }
    reqTrace, err := httputil.DumpRequestOut(req, false)
    if err != nil {
        log.Fatalln(err)
    }
    respTrace, err := httputil.DumpResponse(resp, false)
    if err != nil {
        log.Fatalln(err)
    }
    fmt.Println(string(reqTrace))
    fmt.Println(string(respTrace))
}
$ go run http-test.go
GET / HTTP/1.1
Host: www.google.com
User-Agent: Go-http-client/1.1
Accept-Encoding: gzip


HTTP/1.1 200 OK
Transfer-Encoding: chunked
Alt-Svc: quic="www.google.com:443"; ma=600; v="30,29,28,27,26,25",quic=":443"; ma=600; v="30,29,28,27,26,25"
Alternate-Protocol: 443:quic,p=1
Cache-Control: private, max-age=0
Content-Type: text/html; charset=ISO-8859-1
Date: Sun, 17 Jan 2016 10:11:35 GMT
Expires: -1
P3p: CP="This is not a P3P policy! See https://www.google.com/support/accounts/answer/151657?hl=en for more info."
Server: gws
Set-Cookie: NID=75=zVFTFbi8WLNy04TQUSBVFJ6ioFJs-YUrYLI3Js2dZ8cIoiBLLcxAnOOd4REY9Lns3TPZtPjXE5Cdb7C40vqk4HnbDWdSkSp3j0mMc1Z83pFWR22KMvJcofKJJjOVeKv9GZsJ66MRF1vlxGQ; expires=Mon, 18-Jul-2016 10:11:35 GMT; path=/; domain=.google.com; HttpOnly
X-Frame-Options: SAMEORIGIN
X-Xss-Protection: 1; mode=block
@harshavardhana

This comment has been minimized.

Copy link
Contributor Author

commented Jan 17, 2016

After looking into the code i see that fix to be

diff --git a/src/net/http/request.go b/src/net/http/request.go
index c2f5f26..446a490 100644
--- a/src/net/http/request.go
+++ b/src/net/http/request.go
@@ -413,7 +413,7 @@ func (req *Request) write(w io.Writer, usingProxy bool, extraHeaders Header, wai
                w = bw
        }

-       _, err := fmt.Fprintf(w, "%s %s HTTP/1.1\r\n", valueOrDefault(req.Method, "GET"), ruri)
+       _, err := fmt.Fprintf(w, "%s %s %s\r\n", valueOrDefault(req.Method, "GET"), ruri, valueOrDefault(req.Proto, "HTTP/1.1"))
        if err != nil {
                return err
        }
@harshavardhana

This comment has been minimized.

Copy link
Contributor Author

commented Jan 17, 2016

and testing the program again.

$ ../bin/go run http-test.go
GET / HTTP/2.0
Host: www.google.com
User-Agent: Go-http-client/1.1
Accept-Encoding: gzip


HTTP/2.0 200 OK
Connection: close
Alt-Svc: quic="www.google.com:443"; ma=600; v="30,29,28,27,26,25",quic=":443"; ma=600; v="30,29,28,27,26,25"
Alternate-Protocol: 443:quic,p=1
Cache-Control: private, max-age=0
Content-Type: text/html; charset=ISO-8859-1
Date: Sun, 17 Jan 2016 10:14:15 GMT
Expires: -1
P3p: CP="This is not a P3P policy! See https://www.google.com/support/accounts/answer/151657?hl=en for more info."
Server: gws
Set-Cookie: NID=75=WysA4QXJvCMHyi8lQb0olN4eyV1CSxNsO3UjCi1XZ2YRAR2RxUAztVbc67OD_7paNMvjR-skJ8qs-fNPut86CthxCzUOKR0lobBssfxJes6EeS7V2KwCwfbr1fBcwOi-t-xoXpd_Lrw4Ow; expires=Mon, 18-Jul-2016 10:14:15 GMT; path=/; domain=.google.com; HttpOnly
X-Frame-Options: SAMEORIGIN
X-Xss-Protection: 1; mode=block
@harshavardhana

This comment has been minimized.

Copy link
Contributor Author

commented Jan 17, 2016

Changing to an unsupported server for 2.0

$ cat http-test.go
package main

import (
    "fmt"
    "log"
    "net/http"
    "net/http/httputil"
)

func main() {
    req, err := http.NewRequest("GET", "https://s3.amazonaws.com", nil)
    if err != nil {
        log.Fatalln(err)
    }
    req.Proto = "HTTP/2.0"
    client := &http.Client{}
    resp, err := client.Do(req)
    if err != nil {
        log.Fatalln(err)
    }
    reqTrace, err := httputil.DumpRequestOut(req, false)
    if err != nil {
        log.Fatalln(err)
    }
    respTrace, err := httputil.DumpResponse(resp, false)
    if err != nil {
        log.Fatalln(err)
    }
    fmt.Println(string(reqTrace))
    fmt.Println(string(respTrace))
}
$ ../bin/go run http-test.go
GET / HTTP/2.0
Host: s3.amazonaws.com
User-Agent: Go-http-client/1.1
Accept-Encoding: gzip


HTTP/1.1 505 HTTP Version not supported
Connection: close
Transfer-Encoding: chunked
Content-Type: application/xml
Date: Sun, 17 Jan 16 10:15:35 GMT
X-Amz-Id-2: 6aiu182GZ8ya6mIGI/caABbhy2fYLgi8FQsx7AXQeXzFx2K/ifF55fRz2zgnB9cgA6Y6aQyeEsjU7Hfy1SDpk8y1W/NqkZwN
X-Amz-Request-Id: A7FAF8B8E615F4BE
@gopherbot

This comment has been minimized.

Copy link

commented Jan 17, 2016

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

@nussjustin

This comment has been minimized.

Copy link
Contributor

commented Jan 17, 2016

The comment on Request.Proto says

// The protocol version for incoming requests.
// Client requests always use HTTP/1.1.

So, yeah there is nothing to fix. The code already works as documented. Also this won't work for HTTP 2 anyway as it's implemented in the golang.org/x/net/http2 repo which is vendored into the net/http package.

The comment on Request.Proto should probably be updated to say that HTTP/2.0 will be used for client requests if available.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Jan 17, 2016

The comment actually had been updated. Read at tip.golang.org/pkg/net/http

@bradfitz bradfitz closed this Jan 17, 2016

@bradfitz

This comment has been minimized.

Copy link
Member

commented Jan 17, 2016

I replied further on https://golang.org/cl/18705

@harshavardhana

This comment has been minimized.

Copy link
Contributor Author

commented Jan 17, 2016

The comment actually had been updated. Read at tip.golang.org/pkg/net/http

This is what led to this change. I see the 'keep-alive' change i didn't address.

    // The protocol version for incoming server requests.
    //
    // For client requests these fields are ignored. The HTTP
    // transport code uses either HTTP/1.1 or HTTP/2.0 by default,
    // depending on what the server supports.
    Proto      string // "HTTP/1.0"
    ProtoMajor int    // 1
    ProtoMinor int    // 0

@bradfitz

This comment has been minimized.

Copy link
Member

commented Jan 17, 2016

I don't understand.

@harshavardhana

This comment has been minimized.

Copy link
Contributor Author

commented Jan 17, 2016

I don't understand.

`Proto string // "HTTP/1.0"`` is never honored.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Jan 17, 2016

What about it? That's a valid example value you might get for server requests.

@harshavardhana

This comment has been minimized.

Copy link
Contributor Author

commented Jan 17, 2016

What about it? That's a valid example value you might get for server requests.

Now i see that i didn't read the comment properly, since client is supposed to automatically handle the incoming requests based on their proto. One of the reasons why one would ignore .Proto field on client side.

That was my confusion, thanks for clarifying.

But is it ever valid to set .Proto after http.NewRequest to some value but the underlying write() is always defaulting to HTTP/1.1 - just wondering if this behavior is correct?

@bradfitz

This comment has been minimized.

Copy link
Member

commented Jan 17, 2016

"For client requests these fields are ignored." I don't know how I can word that more clearly.

@harshavardhana

This comment has been minimized.

Copy link
Contributor Author

commented Jan 17, 2016

"For client requests these fields are ignored." I don't know how I can word that more clearly.

Hmm, okay. Understood.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Jan 18, 2016

I actually did think how I can word it more clearly. I sent out https://go-review.googlesource.com/18720

@gopherbot

This comment has been minimized.

Copy link

commented Jan 18, 2016

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

@harshavardhana

This comment has been minimized.

Copy link
Contributor Author

commented Jan 18, 2016

I actually did think how I can word it more clearly. I sent out https://go-review.googlesource.com/18720

Thanks.

gopherbot pushed a commit that referenced this issue Jan 18, 2016

net/http: clarify docs on Request.Proto field
No need to say "by default" because there is no alternative and no way
to override. Always HTTP/2.0 is officially spelled HTTP/2 these days.

Fixes #13985 harder

Change-Id: Ib1ec03cec171ca865342b8e7452cd4c707d7b770
Reviewed-on: https://go-review.googlesource.com/18720
Reviewed-by: Rob Pike <r@golang.org>

@golang golang locked and limited conversation to collaborators Jan 17, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.