code review 3794041: Proxy support for http package. and unit test.

http: add proxy support
Fixes issue 53.

[mattn, 2010-12-20 05:59:29]

Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),

I'd like you to review this change.

[agl1, 2010-12-20 15:51:18]

http://codereview.appspot.com/3794041/diff/2001/src/pkg/http/client.go
File src/pkg/http/client.go (right):

http://codereview.appspot.com/3794041/diff/2001/src/pkg/http/client.go#newcode79
src/pkg/http/client.go:79: info := req.URL.RawUserinfo
You're taking the login information from the target URL, not the proxy URL. Was
that intended?

http://codereview.appspot.com/3794041/diff/2001/src/pkg/http/client.go#newcod...
src/pkg/http/client.go:117: cf := &tls.Config{Rand: rand.Reader, Time:
time.Nanoseconds}
You should be able to pass in a nil config to get the defaults. (If that doesn't
work, please let me know as it should.)

http://codereview.appspot.com/3794041/diff/2001/src/pkg/http/client.go#newcod...
src/pkg/http/client.go:125: proxy_conn.Read(b)
This appears to be assuming that a) the CONNECT is successful and b) that the
full HTTP response is returned in a single read. Neither strike me as safe
assumptions.

[mattn, 2010-12-21 00:18:17]

Hello golang-dev@googlegroups.com, agl1 (cc: golang-dev@googlegroups.com),

Please take another look.

[mattn, 2010-12-21 00:19:51]

Thanks for your review.

I updated. And about your suggestion about 'tls.Config', it's working well.
Thanks.

On 2010/12/20 15:51:18, agl1 wrote:
> http://codereview.appspot.com/3794041/diff/2001/src/pkg/http/client.go
> File src/pkg/http/client.go (right):
> 
>
http://codereview.appspot.com/3794041/diff/2001/src/pkg/http/client.go#newcode79
> src/pkg/http/client.go:79: info := req.URL.RawUserinfo
> You're taking the login information from the target URL, not the proxy URL.
Was
> that intended?
> 
>
http://codereview.appspot.com/3794041/diff/2001/src/pkg/http/client.go#newcod...
> src/pkg/http/client.go:117: cf := &tls.Config{Rand: rand.Reader, Time:
> time.Nanoseconds}
> You should be able to pass in a nil config to get the defaults. (If that
doesn't
> work, please let me know as it should.)
> 
>
http://codereview.appspot.com/3794041/diff/2001/src/pkg/http/client.go#newcod...
> src/pkg/http/client.go:125: proxy_conn.Read(b)
> This appears to be assuming that a) the CONNECT is successful and b) that the
> full HTTP response is returned in a single read. Neither strike me as safe
> assumptions.

[jacekm, 2010-12-21 00:30:20]

http://codereview.appspot.com/3794041/diff/2001/src/pkg/http/client.go
File src/pkg/http/client.go (right):

http://codereview.appspot.com/3794041/diff/2001/src/pkg/http/client.go#newcode67
src/pkg/http/client.go:67: if err == nil {
Reversing the logic (err != nil) would save one indentation level.

http://codereview.appspot.com/3794041/diff/2001/src/pkg/http/client.go#newcode72
src/pkg/http/client.go:72: no_proxies := strings.Split(no_proxy, ",", -1)
It's no less readable to range over strings.Split than to indirect through
no_proxies.

http://codereview.appspot.com/3794041/diff/2001/src/pkg/http/client.go#newcode74
src/pkg/http/client.go:74: if strings.Index(addr, no_proxy) != -1 {
Too broad.  If no_proxy="bar.com", then both bar.com and foobar.com will be
contacted directly.  While fixing, keep in mind that addr contains port.

http://codereview.appspot.com/3794041/diff/2001/src/pkg/http/client.go#newcod...
src/pkg/http/client.go:127: conn.(*tls.Conn).Handshake()
tls.Handshake returns os.Error, please check it.

http://codereview.appspot.com/3794041/diff/2001/src/pkg/http/client.go#newcod...
src/pkg/http/client.go:144: if err :=
conn.(*tls.Conn).VerifyHostname(req.URL.Host); err != nil {
Why verify req.URL.Host and not h like before?

[mattn, 2010-12-21 02:03:41]

http://codereview.appspot.com/3794041/diff/2001/src/pkg/http/client.go
File src/pkg/http/client.go (right):

http://codereview.appspot.com/3794041/diff/2001/src/pkg/http/client.go#newcode72
src/pkg/http/client.go:72: no_proxies := strings.Split(no_proxy, ",", -1)
Thanks for your review.

Do you mean that this should check maximum count of no_proxy?

On 2010/12/21 00:30:20, jacekm wrote:
> It's no less readable to range over strings.Split than to indirect through
> no_proxies.

[mattn, 2010-12-21 02:58:04]

Hello golang-dev@googlegroups.com, agl1, jacek.masiulaniec@gmail.com (cc:
golang-dev@googlegroups.com),

Please take another look.

[mattn, 2010-12-21 03:00:17]

Hello golang-dev@googlegroups.com, agl1, jacek.masiulaniec@gmail.com (cc:
golang-dev@googlegroups.com),

Please take another look.

[mattn, 2010-12-21 03:07:13]

Hello golang-dev@googlegroups.com, agl1, jacek.masiulaniec@gmail.com (cc:
golang-dev@googlegroups.com),

Please take another look.

[mattn, 2010-12-21 03:08:26]

Hello golang-dev@googlegroups.com, agl1, jacek.masiulaniec@gmail.com (cc:
golang-dev@googlegroups.com),

Please take another look.

[mattn, 2010-12-21 03:09:29]

Hello golang-dev@googlegroups.com, agl1, jacek.masiulaniec@gmail.com (cc:
golang-dev@googlegroups.com),

Please take another look.

[mattn, 2010-12-21 03:09:55]

Thanks a lot!

I updated.

On 2010/12/21 00:30:20, jacekm wrote:
> http://codereview.appspot.com/3794041/diff/2001/src/pkg/http/client.go
> File src/pkg/http/client.go (right):
> 
>
http://codereview.appspot.com/3794041/diff/2001/src/pkg/http/client.go#newcode67
> src/pkg/http/client.go:67: if err == nil {
> Reversing the logic (err != nil) would save one indentation level.
> 
>
http://codereview.appspot.com/3794041/diff/2001/src/pkg/http/client.go#newcode72
> src/pkg/http/client.go:72: no_proxies := strings.Split(no_proxy, ",", -1)
> It's no less readable to range over strings.Split than to indirect through
> no_proxies.
> 
>
http://codereview.appspot.com/3794041/diff/2001/src/pkg/http/client.go#newcode74
> src/pkg/http/client.go:74: if strings.Index(addr, no_proxy) != -1 {
> Too broad.  If no_proxy="bar.com", then both http://bar.com and
http://foobar.com will be
> contacted directly.  While fixing, keep in mind that addr contains port.
> 
>
http://codereview.appspot.com/3794041/diff/2001/src/pkg/http/client.go#newcod...
> src/pkg/http/client.go:127: conn.(*tls.Conn).Handshake()
> tls.Handshake returns os.Error, please check it.
> 
>
http://codereview.appspot.com/3794041/diff/2001/src/pkg/http/client.go#newcod...
> src/pkg/http/client.go:144: if err :=
> conn.(*tls.Conn).VerifyHostname(req.URL.Host); err != nil {
> Why verify req.URL.Host and not h like before?

[adg, 2010-12-21 03:26:38]

Some tweaks, and a question about behavior. Thanks.

http://codereview.appspot.com/3794041/diff/21001/src/pkg/http/client.go
File src/pkg/http/client.go (right):

http://codereview.appspot.com/3794041/diff/21001/src/pkg/http/client.go#newco...
src/pkg/http/client.go:41: no_proxies := strings.Split(no_proxy, ",", -1)
Might as well put the strings.Split in the range statement below and avoid
creating no_proxies at all.

http://codereview.appspot.com/3794041/diff/21001/src/pkg/http/client.go#newco...
src/pkg/http/client.go:43: if hasPort(no_proxy) {
This is and the following 2 lines are duplicate code. Remove them.

http://codereview.appspot.com/3794041/diff/21001/src/pkg/http/client.go#newco...
src/pkg/http/client.go:46: for _, no_proxy := range no_proxies {
s/no_proxy/p/ - the code will look cleaner

http://codereview.appspot.com/3794041/diff/21001/src/pkg/http/client.go#newco...
src/pkg/http/client.go:52: (no_proxy[0] == '.' && "."+addr == no_proxy) ||
instead of this and the following expressions, why not strip off the . before
the if statement?

if p[0] == ".' { p = p[1:] }

That will preserve the logic you have already, but I'm not sure if the logic is
sound.

If a NO_PROXY domain begins with a period, should subdomains of that domain be
excluded also? For example, for
 NO_PROXY=.example.net
should
 foo.example.net
match?

http://codereview.appspot.com/3794041/diff/21001/src/pkg/http/client.go#newco...
src/pkg/http/client.go:92: return nil, os.ErrorString("invalid proxy address")
Maybe instead of failing outright, this should log a message to the console and
proceed without using a proxy.

http://codereview.appspot.com/3794041/diff/21001/src/pkg/http/client.go#newco...
src/pkg/http/client.go:99: info = proxy_url.RawUserinfo
s/info/proxy_info/

http://codereview.appspot.com/3794041/diff/21001/src/pkg/http/client.go#newco...
src/pkg/http/client.go:108: if req.URL.Scheme == "http" {
maybe these nested if statements should be replaced with:

switch {
case req.URL.Scheme == "http" && proxy_url != nil:
case req.URL.Scheme == "http": // no proxy
case proxy_url != nil: // https with proxy
default: // https
}

http://codereview.appspot.com/3794041/diff/21001/src/pkg/http/proxy_test.go
File src/pkg/http/proxy_test.go (right):

http://codereview.appspot.com/3794041/diff/21001/src/pkg/http/proxy_test.go#n...
src/pkg/http/proxy_test.go:25: {"foofoobar.com", false},
It's difficult for me to parse the intention behind these test cases. Start with
the valid ones, then move onto groups of variants. Comment on the failure cases.

[mattn, 2010-12-21 03:48:59]

Hello golang-dev@googlegroups.com, agl1, jacek.masiulaniec@gmail.com, adg (cc:
golang-dev@googlegroups.com),

Please take another look.

[mattn, 2010-12-21 03:49:45]

Hello golang-dev@googlegroups.com, agl1, jacek.masiulaniec@gmail.com, adg (cc:
golang-dev@googlegroups.com),

Please take another look.

[mattn, 2010-12-21 03:49:47]

http://codereview.appspot.com/3794041/diff/21001/src/pkg/http/client.go
File src/pkg/http/client.go (right):

http://codereview.appspot.com/3794041/diff/21001/src/pkg/http/client.go#newco...
src/pkg/http/client.go:43: if hasPort(no_proxy) {
On 2010/12/21 03:26:38, adg wrote:
> This is and the following 2 lines are duplicate code. Remove them.

oops

http://codereview.appspot.com/3794041/diff/21001/src/pkg/http/client.go#newco...
src/pkg/http/client.go:52: (no_proxy[0] == '.' && "."+addr == no_proxy) ||
I think that following should be match.
addr             no_proxy
foobar.com     = foobar.com
foobar.com     = .foobar.com
www.foobar.com = .foobar.com

If strip before, below get mis-matching.

barfoobar.com = .foobar.com

On 2010/12/21 03:26:38, adg wrote:
> instead of this and the following expressions, why not strip off the . before
> the if statement?
> 
> if p[0] == ".' { p = p[1:] }
> 
> That will preserve the logic you have already, but I'm not sure if the logic
is
> sound.
> 
> If a NO_PROXY domain begins with a period, should subdomains of that domain be
> excluded also? For example, for
>  http://NO_PROXY=.example.net
> should
>  http://foo.example.net
> match?

http://codereview.appspot.com/3794041/diff/21001/src/pkg/http/client.go#newco...
src/pkg/http/client.go:92: return nil, os.ErrorString("invalid proxy address")
Hmm, I don't think so.
'curl' give an error message and stop when proxy url was wrong.

On 2010/12/21 03:26:38, adg wrote:
> Maybe instead of failing outright, this should log a message to the console
and
> proceed without using a proxy.

[mattn, 2010-12-21 04:08:39]

Hello golang-dev@googlegroups.com, agl1, jacek.masiulaniec@gmail.com, adg (cc:
golang-dev@googlegroups.com),

Please take another look.

[mattn, 2010-12-21 04:39:08]

Hello golang-dev@googlegroups.com, agl1, jacek.masiulaniec@gmail.com, adg (cc:
golang-dev@googlegroups.com),

Please take another look.

[mattn, 2010-12-21 04:39:50]

Thanks for your review(s).
And sorry about my many posts.

On 2010/12/21 04:39:08, mattn wrote:
> Hello mailto:golang-dev@googlegroups.com, agl1,
mailto:jacek.masiulaniec@gmail.com, adg (cc:
> mailto:golang-dev@googlegroups.com),
> 
> Please take another look.

[jacekm, 2010-12-21 20:38:06]

http://codereview.appspot.com/3794041/diff/47001/src/pkg/http/client.go
File src/pkg/http/client.go (right):

http://codereview.appspot.com/3794041/diff/47001/src/pkg/http/client.go#newco...
src/pkg/http/client.go:36: func matchNoProxy(addr string) bool {
Others additionally implement special case no_proxy="*", which prevents use of
any proxy.

http://codereview.appspot.com/3794041/diff/47001/src/pkg/http/client.go#newco...
src/pkg/http/client.go:42: tmp := strings.TrimSpace(addr)
It would be more consistent with below to drop tmp and recycle addr instead,
i.e. addr = strings.TrimSpace(addr)

http://codereview.appspot.com/3794041/diff/47001/src/pkg/http/client.go#newco...
src/pkg/http/client.go:44: tmp = tmp[0:strings.LastIndex(tmp, ":")]
The port shall not be ignored.  It's required to implement support for e.g.
no_proxy="foo.com:8080", which is missing.

http://codereview.appspot.com/3794041/diff/47001/src/pkg/http/client.go#newco...
src/pkg/http/client.go:50: p = p[0:strings.LastIndex(p, ":")]
Same.

http://codereview.appspot.com/3794041/diff/47001/src/pkg/http/client.go#newco...
src/pkg/http/client.go:54: (p[0] == '.' && tmp == p[1:]) {
Can you give example no_proxy which requires this code?

http://codereview.appspot.com/3794041/diff/47001/src/pkg/http/client.go#newco...
src/pkg/http/client.go:137: proxy_conn.Write([]byte("CONNECT " + addr + "
HTTP/1.0\r\n"))
Are you certain it's correct to send "HTTP/1.0" unconditionally?  Is it correct
in every case?

http://codereview.appspot.com/3794041/diff/47001/src/pkg/http/client.go#newco...
src/pkg/http/client.go:142: proxy_conn.Read(b)
As mentioned by agl, this and preceding lines are incorrect in few ways.

[jacekm, 2010-12-21 20:42:52]

http://codereview.appspot.com/3794041/diff/47001/src/pkg/http/client.go
File src/pkg/http/client.go (right):

http://codereview.appspot.com/3794041/diff/47001/src/pkg/http/client.go#newco...
src/pkg/http/client.go:36: func matchNoProxy(addr string) bool {
Also, support of http_proxy and no_proxy should be documented.

[mattn, 2010-12-22 00:30:50]

http://codereview.appspot.com/3794041/diff/47001/src/pkg/http/client.go
File src/pkg/http/client.go (right):

http://codereview.appspot.com/3794041/diff/47001/src/pkg/http/client.go#newco...
src/pkg/http/client.go:54: (p[0] == '.' && tmp == p[1:]) {
On 2010/12/21 20:38:07, jacekm wrote:
> Can you give example no_proxy which requires this code?

Ah, I was confused. I'll fix soon

[mattn, 2010-12-22 00:58:33]

http://codereview.appspot.com/3794041/diff/47001/src/pkg/http/client.go
File src/pkg/http/client.go (right):

http://codereview.appspot.com/3794041/diff/47001/src/pkg/http/client.go#newco...
src/pkg/http/client.go:137: proxy_conn.Write([]byte("CONNECT " + addr + "
HTTP/1.0\r\n"))
On 2010/12/21 20:38:07, jacekm wrote:
> Are you certain it's correct to send "HTTP/1.0" unconditionally?  Is it
correct
> in every case?

I read RFC2817. but I can't understand why Host: is needed.

[mattn, 2010-12-22 01:04:15]

http://codereview.appspot.com/3794041/diff/47001/src/pkg/http/client.go
File src/pkg/http/client.go (right):

http://codereview.appspot.com/3794041/diff/47001/src/pkg/http/client.go#newco...
src/pkg/http/client.go:142: proxy_conn.Read(b)
On 2010/12/21 20:38:07, jacekm wrote:
> As mentioned by agl, this and preceding lines are incorrect in few ways.

I understood that agl said about error handling. then I added the code. I may
have mis-understanding. X-(

[mattn, 2010-12-22 01:04:38]

Hello golang-dev@googlegroups.com, agl1, jacek.masiulaniec@gmail.com, adg (cc:
golang-dev@googlegroups.com),

Please take another look.

[mattn, 2010-12-22 01:19:52]

Hello golang-dev@googlegroups.com, agl1, jacek.masiulaniec@gmail.com, adg (cc:
golang-dev@googlegroups.com),

Please take another look.

[mattn, 2010-12-30 17:38:12]

I'm looking for any replies or taking another look. :)

On 2010/12/22 01:19:52, mattn wrote:
> Hello mailto:golang-dev@googlegroups.com, agl1,
mailto:jacek.masiulaniec@gmail.com, adg (cc:
> mailto:golang-dev@googlegroups.com),
> 
> Please take another look.

[rsc, 2011-01-04 16:52:56]

http://codereview.appspot.com/3794041/diff/57001/src/pkg/http/client.go
File src/pkg/http/client.go (right):

http://codereview.appspot.com/3794041/diff/57001/src/pkg/http/client.go#newco...
src/pkg/http/client.go:63: p = p[1:]
Is there a spec for how to interpret these environment variables?
I am a little surprised that swtch.com and .swtch.com are interpreted
identically.

http://codereview.appspot.com/3794041/diff/57001/src/pkg/http/client.go#newco...
src/pkg/http/client.go:102: proxy_auth := ""
don't use _ in variable names.
proxyAuth
proxyURL
proxyInfo


no_proxy is okay above because it is the variable name.
but it's an exception.

http://codereview.appspot.com/3794041/diff/57001/src/pkg/http/client.go#newco...
src/pkg/http/client.go:123: switch {
Too much duplication in the cases

if req.URL.Scheme == "http" {
   if proxyURL != nil {
       addr = proxyURL.Host
       req.Header["Proxy-Authorization"] = proxyAuth
   }
   conn, err = net.Dial("tcp", "", addr)
   if err != nil {
        return nil, err
   }
} else {
   // similarly for https
}

[mattn, 2011-01-05 00:39:30]

Hello agl1, jacek.masiulaniec@gmail.com, adg, rsc (cc:
golang-dev@googlegroups.com),

Please take another look.

[mattn, 2011-01-05 00:39:48]

http://codereview.appspot.com/3794041/diff/57001/src/pkg/http/client.go
File src/pkg/http/client.go (right):

http://codereview.appspot.com/3794041/diff/57001/src/pkg/http/client.go#newco...
src/pkg/http/client.go:63: p = p[1:]
I refer the code of cURL.

https://github.com/bagder/curl/blob/master/lib/url.c#L3983

On 2011/01/04 16:52:56, rsc wrote:
> Is there a spec for how to interpret these environment variables?
> I am a little surprised that http://swtch.com and .swtch.com are interpreted
> identically.

http://codereview.appspot.com/3794041/diff/57001/src/pkg/http/client.go#newco...
src/pkg/http/client.go:102: proxy_auth := ""
On 2011/01/04 16:52:56, rsc wrote:
> don't use _ in variable names.
> proxyAuth
> proxyURL
> proxyInfo
> 
> 
> no_proxy is okay above because it is the variable name.
> but it's an exception.

Done.

http://codereview.appspot.com/3794041/diff/57001/src/pkg/http/client.go#newco...
src/pkg/http/client.go:123: switch {
proxyURL may not include authenticate information.
And it is difference of connecting address.

On 2011/01/04 16:52:56, rsc wrote:
> Too much duplication in the cases
> 
> if req.URL.Scheme == "http" {
>    if proxyURL != nil {
>        addr = proxyURL.Host
>        req.Header["Proxy-Authorization"] = proxyAuth
>    }
>    conn, err = net.Dial("tcp", "", addr)
>    if err != nil {
>         return nil, err
>    }
> } else {
>    // similarly for https
> }
>

[rsc, 2011-01-19 20:21:11]

http://codereview.appspot.com/3794041/diff/57001/src/pkg/http/client.go
File src/pkg/http/client.go (right):

http://codereview.appspot.com/3794041/diff/57001/src/pkg/http/client.go#newco...
src/pkg/http/client.go:123: switch {
On 2011/01/05 00:39:48, mattn wrote:
> proxyURL may not include authenticate information.
> And it is difference of connecting address.

It should still be possible to eliminate some of the duplication.

[mattn, 2011-02-03 02:38:25]

http://codereview.appspot.com/3794041/diff/57001/src/pkg/http/client.go
File src/pkg/http/client.go (right):

http://codereview.appspot.com/3794041/diff/57001/src/pkg/http/client.go#newco...
src/pkg/http/client.go:123: switch {
Do you say about L125 and L138 ?
Sorry please give me a description.

On 2011/01/19 20:21:11, rsc wrote:
> On 2011/01/05 00:39:48, mattn wrote:
> > proxyURL may not include authenticate information.
> > And it is difference of connecting address.
> 
> It should still be possible to eliminate some of the duplication.

[rsc, 2011-02-09 05:25:02]

http://codereview.appspot.com/3794041/diff/66001/src/pkg/http/client.go
File src/pkg/http/client.go (right):

http://codereview.appspot.com/3794041/diff/66001/src/pkg/http/client.go#newco...
src/pkg/http/client.go:99: if len(proxy) == 0 {
this makes me think proxy is a list.  use proxy == "" instead.
it compiles to the same thing.

http://codereview.appspot.com/3794041/diff/66001/src/pkg/http/client.go#newco...
src/pkg/http/client.go:103: proxyURL, err := ParseURL(proxy)
Does this still work without a proxy?
What does ParseURL("") do here?

http://codereview.appspot.com/3794041/diff/66001/src/pkg/http/client.go#newco...
src/pkg/http/client.go:108: if matchNoProxy(addr) {
Should check this before all the work on proxy.
If http_proxy is set to garbage but matchNoProxy is true,
then the call should succeed.

http://codereview.appspot.com/3794041/diff/66001/src/pkg/http/client.go#newco...
src/pkg/http/client.go:124: case req.URL.Scheme == "http" && proxyURL != nil: //
http via proxy
I am serious about the duplicate code here.
The dial is duplicated, but worse the TLS verification logic is duplicated.
Here is non-duplicated code:

// Connect to server or proxy.
if proxyURL != nil {
    addr = proxyURL.Host
}
conn, err = net.Dial("tcp", "", addr)
if err != nil {
    return nil, err
}

if req.URL.Scheme == "http" {
    // Include proxy http header if needed.
    if proxyAuth != "" {
        req.Header["Proxy-Authorization"] = proxyAuth
    }
} else { // https
    if proxyURL != nil {
        // Ask proxy for direct connection to server.
        // addr defaults above to ":https" but we need to use numbers
        addr = req.URL.Host
        if !hasPort(addr) {
            addr += ":443"
        }
        fmt.Fprintf(conn, "CONNECT %s HTTP/1.1\r\n", addr)
        fmt.Fprintf(conn, "Host: %s\r\n", addr)
        if proxyAuth != "" {
            fmt.Fprintf(conn, "Proxy-Authorization: %s\r\n", proxyAuth)
        }
        fmt.Fprintf(conn, "\r\n")
        
        // Read response.
        // Okay to use and discard buffered reader here, because
        // TLS server will not speak until spoken to.
        br := bufio.NewReader(conn)
        resp, err := ReadResponse(br, "CONNECT")
        ... check resp ...
    }

    // Initiate TLS and check remote host name against certificate.
    conn = tls.Client(conn, nil)
    if err = conn.(*tls.Conn).Handshake(); err != nil {
        return nil, err
    }
    h := req.URL.Host
    if hasPort(h) {
        h = h[:strings.LastIndex(h, ":")]
    }
    if err = conn.(*tls.Conn).VerifyHostname(h); err != nil {
        return nil, err
    }
}

[mattn, 2011-02-09 06:59:25]

Hello agl1, jacek.masiulaniec@gmail.com, adg, rsc (cc:
golang-dev@googlegroups.com),

I'd like you to review this change to
http://go.googlecode.com/hg/

[mattn, 2011-02-09 07:00:03]

Thanks. code become cleanly.

http://codereview.appspot.com/3794041/diff/66001/src/pkg/http/client.go
File src/pkg/http/client.go (right):

http://codereview.appspot.com/3794041/diff/66001/src/pkg/http/client.go#newco...
src/pkg/http/client.go:99: if len(proxy) == 0 {
On 2011/02/09 05:25:03, rsc wrote:
> this makes me think proxy is a list.  use proxy == "" instead.
> it compiles to the same thing.
> 

Done.

http://codereview.appspot.com/3794041/diff/66001/src/pkg/http/client.go#newco...
src/pkg/http/client.go:103: proxyURL, err := ParseURL(proxy)
On 2011/02/09 05:25:03, rsc wrote:
> Does this still work without a proxy?
> What does ParseURL("") do here?

Done.

http://codereview.appspot.com/3794041/diff/66001/src/pkg/http/client.go#newco...
src/pkg/http/client.go:108: if matchNoProxy(addr) {
On 2011/02/09 05:25:03, rsc wrote:
> Should check this before all the work on proxy.
> If http_proxy is set to garbage but matchNoProxy is true,
> then the call should succeed.

Done.

http://codereview.appspot.com/3794041/diff/66001/src/pkg/http/client.go#newco...
src/pkg/http/client.go:124: case req.URL.Scheme == "http" && proxyURL != nil: //
http via proxy
On 2011/02/09 05:25:03, rsc wrote:
> I am serious about the duplicate code here.
> The dial is duplicated, but worse the TLS verification logic is duplicated.
> Here is non-duplicated code:
> 
> // Connect to server or proxy.
> if proxyURL != nil {
>     addr = proxyURL.Host
> }
> conn, err = net.Dial("tcp", "", addr)
> if err != nil {
>     return nil, err
> }
> 
> if req.URL.Scheme == "http" {
>     // Include proxy http header if needed.
>     if proxyAuth != "" {
>         req.Header["Proxy-Authorization"] = proxyAuth
>     }
> } else { // https
>     if proxyURL != nil {
>         // Ask proxy for direct connection to server.
>         // addr defaults above to ":https" but we need to use numbers
>         addr = req.URL.Host
>         if !hasPort(addr) {
>             addr += ":443"
>         }
>         fmt.Fprintf(conn, "CONNECT %s HTTP/1.1\r\n", addr)
>         fmt.Fprintf(conn, "Host: %s\r\n", addr)
>         if proxyAuth != "" {
>             fmt.Fprintf(conn, "Proxy-Authorization: %s\r\n", proxyAuth)
>         }
>         fmt.Fprintf(conn, "\r\n")
>         
>         // Read response.
>         // Okay to use and discard buffered reader here, because
>         // TLS server will not speak until spoken to.
>         br := bufio.NewReader(conn)
>         resp, err := ReadResponse(br, "CONNECT")
>         ... check resp ...
>     }
> 
>     // Initiate TLS and check remote host name against certificate.
>     conn = tls.Client(conn, nil)
>     if err = conn.(*tls.Conn).Handshake(); err != nil {
>         return nil, err
>     }
>     h := req.URL.Host
>     if hasPort(h) {
>         h = h[:strings.LastIndex(h, ":")]
>     }
>     if err = conn.(*tls.Conn).VerifyHostname(h); err != nil {
>         return nil, err
>     }
> }
> 

Done.

[rsc, 2011-02-10 19:03:32]

The code looks great, thanks.

The CL description says "and unit test".
Maybe you need to run hg change 3794041
to add the unit test file to the list of changed files?

[mattn, 2011-02-13 14:56:55]

Hello agl1, jacek.masiulaniec@gmail.com, adg, rsc (cc:
golang-dev@googlegroups.com),

I'd like you to review this change to
https://go.googlecode.com/hg/

[rsc, 2011-02-14 17:17:10]

http://codereview.appspot.com/3794041/diff/76001/src/pkg/http/proxy_test.go
File src/pkg/http/proxy_test.go (right):

http://codereview.appspot.com/3794041/diff/76001/src/pkg/http/proxy_test.go#n...
src/pkg/http/proxy_test.go:21: var matchNoProxyTests = []struct {
this should be a global variable (at top level)

http://codereview.appspot.com/3794041/diff/76001/src/pkg/http/proxy_test.go#n...
src/pkg/http/proxy_test.go:38: if test.match {
t.Errorf("matchNoProxy(%v) = %v, want %v", test.host, !test.match, test.match)

[agl1, 2011-02-14 17:35:25]

(Note: you have too many people listed as reviewers.)

http://codereview.appspot.com/3794041/diff/76001/src/pkg/http/client.go
File src/pkg/http/client.go (right):

http://codereview.appspot.com/3794041/diff/76001/src/pkg/http/client.go#newco...
src/pkg/http/client.go:34: // Return whether given a string matches as NO_PROXY.
if true, it use
matchNoProxy returns true if requests to addr should not use a proxy, according
to the NO_PROXY or no_proxy environment variable.

http://codereview.appspot.com/3794041/diff/76001/src/pkg/http/client.go#newco...
src/pkg/http/client.go:54: for _, p := range strings.Split(no_proxy, ",", -1) {
it would seem that a lot of work only needs to be done once per process. But
that's not critical for a first patch.

http://codereview.appspot.com/3794041/diff/76001/src/pkg/http/client.go#newco...
src/pkg/http/client.go:65: if addr == p ||
I would make this one line.

http://codereview.appspot.com/3794041/diff/76001/src/pkg/http/client.go#newco...
src/pkg/http/client.go:66: (strings.HasPrefix(addr, p) && len(addr) > len(p) &&
addr[len(p)-1] == '.') {
This seems odd. Shouldn't it be HasSuffix? I would expect |p| to contain
something like "example.com" and |addr| to contain "foo.example.com"

http://codereview.appspot.com/3794041/diff/76001/src/pkg/http/client.go#newco...
src/pkg/http/client.go:113: if proxyURL != nil {
In what case does ParseURL return a nil proxyURL with a nil error?

[rsc, 2011-02-14 17:37:47]

On Mon, Feb 14, 2011 at 12:35,  <agl@golang.org> wrote:
> (Note: you have too many people listed as reviewers.)

Replying to the review thread automatically adds your name.
You replied at some point last week.

Russ

[mattn, 2011-02-15 02:04:55]

http://codereview.appspot.com/3794041/diff/76001/src/pkg/http/client.go
File src/pkg/http/client.go (right):

http://codereview.appspot.com/3794041/diff/76001/src/pkg/http/client.go#newco...
src/pkg/http/client.go:34: // Return whether given a string matches as NO_PROXY.
if true, it use
On 2011/02/14 17:35:25, agl1 wrote:
> matchNoProxy returns true if requests to addr should not use a proxy,
according
> to the NO_PROXY or no_proxy environment variable.

Done.

http://codereview.appspot.com/3794041/diff/76001/src/pkg/http/client.go#newco...
src/pkg/http/client.go:54: for _, p := range strings.Split(no_proxy, ",", -1) {
Hmm, Yes I see.
But I think also that we should provide way to set/reset NO_PROXY/HTTP_PROXY
dynamicaly each processes.

Or we have better to provide APIs for proxy?

On 2011/02/14 17:35:25, agl1 wrote:
> it would seem that a lot of work only needs to be done once per process. But
> that's not critical for a first patch.

http://codereview.appspot.com/3794041/diff/76001/src/pkg/http/client.go#newco...
src/pkg/http/client.go:65: if addr == p ||
On 2011/02/14 17:35:25, agl1 wrote:
> I would make this one line.

Done.

http://codereview.appspot.com/3794041/diff/76001/src/pkg/http/client.go#newco...
src/pkg/http/client.go:66: (strings.HasPrefix(addr, p) && len(addr) > len(p) &&
addr[len(p)-1] == '.') {
"NO_PROXY=foobar.com" should not match to "bar.com".
But code I writen is so bad. I'll fix it. Thanks for notice.

http://codereview.appspot.com/3794041/diff/76001/src/pkg/http/client.go#newco...
src/pkg/http/client.go:113: if proxyURL != nil {
On 2011/02/14 17:35:25, agl1 wrote:
> In what case does ParseURL return a nil proxyURL with a nil error?

Done.

http://codereview.appspot.com/3794041/diff/76001/src/pkg/http/proxy_test.go
File src/pkg/http/proxy_test.go (right):

http://codereview.appspot.com/3794041/diff/76001/src/pkg/http/proxy_test.go#n...
src/pkg/http/proxy_test.go:21: var matchNoProxyTests = []struct {
On 2011/02/14 17:17:10, rsc wrote:
> this should be a global variable (at top level)

Done.

http://codereview.appspot.com/3794041/diff/76001/src/pkg/http/proxy_test.go#n...
src/pkg/http/proxy_test.go:38: if test.match {
On 2011/02/14 17:17:10, rsc wrote:
> t.Errorf("matchNoProxy(%v) = %v, want %v", test.host, !test.match, test.match)

Done.

[mattn, 2011-02-15 02:05:04]

Hello agl1, jacek.masiulaniec@gmail.com, adg, rsc (cc:
golang-dev@googlegroups.com),

Please take another look.

[mattn, 2011-02-15 02:05:22]

Hello agl1, jacek.masiulaniec@gmail.com, adg, rsc (cc:
golang-dev@googlegroups.com),

Please take another look.

[agl, 2011-02-15 14:44:32]

LGTM. I'll land it when you're ready but I'll let you clean up that comment if
you wish.

http://codereview.appspot.com/3794041/diff/75002/src/pkg/http/client.go
File src/pkg/http/client.go (right):

http://codereview.appspot.com/3794041/diff/75002/src/pkg/http/client.go#newco...
src/pkg/http/client.go:36: // if true, it use HTTP_PROXY. This variable should
be checked with
These final two lines of comments should either be removed, or better written.

[mattn, 2011-02-16 00:06:35]

Thanks for your reviews.

http://codereview.appspot.com/3794041/diff/75002/src/pkg/http/client.go
File src/pkg/http/client.go (right):

http://codereview.appspot.com/3794041/diff/75002/src/pkg/http/client.go#newco...
src/pkg/http/client.go:36: // if true, it use HTTP_PROXY. This variable should
be checked with
On 2011/02/15 14:44:32, agl wrote:
> These final two lines of comments should either be removed, or better written.

Done.

[rsc, 2011-02-16 04:50:19]

Please change the CL description to

    http: add proxy support
    Fixes issue 53.

thanks.

[rsc, 2011-02-16 06:08:14]

> How do I have to do 'hg XXX' to change CL's description.
> I couldn't 'hg change 3794041'. (started vim, and :wq, but not changes)
> Thanks.

hg change should work, but it will not if you have
been using hg clpatch to move the CL between machines.
Another way to edit the description is to use the web page.
Go to http://codereview.appspot.com/3794041
and then click "Edit issue".

Russ

[agl1, 2011-02-16 19:07:09]

*** Submitted as http://code.google.com/p/go/source/detail?r=45734a031210 ***

http: add proxy support
Fixes issue 53.

R=agl1, jacek.masiulaniec, adg, rsc, agl
CC=golang-dev
http://codereview.appspot.com/3794041

Committer: Adam Langley <agl@golang.org>