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

http: false User-Agent in DumpRequest #2272

Closed
masiulaniec opened this issue Sep 18, 2011 · 7 comments
Closed

http: false User-Agent in DumpRequest #2272

masiulaniec opened this issue Sep 18, 2011 · 7 comments

Comments

@masiulaniec
Copy link

In documentation the function http.DumpRequest is defined to dump the request's wire
representation.  In reality it makes up some fields.  In below example notice how my
telnet is reported to be a "Go http package".


package main

import  "http"

func dump(w http.ResponseWriter, req *http.Request) {
        dump, _ := http.DumpRequest(req, false)
        w.Write(dump)
}

func main() {
        http.ListenAndServe(":6060", http.HandlerFunc(dump))
}


Connected to localhost.
Escape character is '^]'.
GET / HTTP/1.0

HTTP/1.0 200 OK
Date: Sun, 18 Sep 2011 18:39:54 GMT
Content-Type: text/plain; charset=utf-8

GET / HTTP/1.1
Host: 
User-Agent: Go http package

Connection closed by foreign host.
@masiulaniec
Copy link
Author

Comment 1:

It's also making up HTTP protocol version.

@bradfitz
Copy link
Contributor

Comment 2:

Accepted, but this is mostly a documentation problem more than a bug.
Request.Write is about what you'd write if you were sending this request to another host
as a client, not about preserving the exact bytes you got in.  (hence the HTTP/1.1
version, for example)  The blank host is actually required for HTTP/1.1, if I recall
correctly.  The User-Agent perhaps should be added by the http.Client, rather than
DumpRequest.  I'll think about that part.

Owner changed to @bradfitz.

Status changed to Accepted.

@rsc
Copy link
Contributor

rsc commented Sep 19, 2011

Comment 3:

I think DumpRequest should not add these lines if possible.
That might mean making it a little more than a wrapper
around Request.Write.

@bradfitz
Copy link
Contributor

Comment 4:

Oh, right, this is about DumpRequest more than Request.Write.
I think I've used DumpRequest once ever, but looking at the docs now, and rsc's comment
#3, it's unclear what this should mean:
// DumpRequest returns the wire representation of req
Does that mean the incoming wire representation (what we just got in a Server Handler?)
or the outgoing representation (what a client would send?)
DumpRequest already takes one somewhat ugly bool parameter.  It'd be really gross to add
a second.
Arguably the biggest feature of DumpRequest is its Request.Body saving/cloning. 
Otherwise you could just use Request.Write.  So I assume that this function was intended
to show the *outgoing* wire representation.
Should we rename it to DumpClientRequest and add a DumpServerRequest?
Should we keep the "body bool" parameter or make the client do that?  Perhaps exposing
drainBody as an *http.DumpBodySaver type?  Seems like too much package noise and too
much abstraction for a debug function.
I'm happy to do whatever if people have opinions.

@rsc
Copy link
Contributor

rsc commented Sep 19, 2011

Comment 5:

I'd prefer to just leave the debug API as it is
and fix DumpRequest.  Because it is "dump",
it should not add or remove things, just print
what's there.
Russ

@bradfitz
Copy link
Contributor

Comment 6:

http://golang.org/cl/5043051

@bradfitz
Copy link
Contributor

Comment 7:

This issue was closed by revision 6b6cb72.

Status changed to Fixed.

@golang golang locked and limited conversation to collaborators Jun 24, 2016
This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants