code review 48870045: net/http: Remove proxy detector for the transport

net/http: Remove proxy detector for the transport
Fixes issue 7020

[mattn, 2014-01-08 06:02:46]

Hello golang-codereviews@googlegroups.com,

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

[mattn, 2014-01-08 06:04:06]

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

Please take another look.

[gobot, 2014-01-08 06:23:31]

R=bradfitz@golang.org (assigned by mattn.jp@gmail.com)

[bradfitz, 2014-01-08 17:41:33]

not lgtm

This is a data race.

[mattn, 2014-01-15 02:22:20]

Do you mean that breaking compatibility of second argument?

[bradfitz, 2014-01-15 03:01:03]

No, I meant that it's a data race: one goroutine is writing to memory that
another goroutine can be reading, without any synchronization used by both
of them.



On Tue, Jan 14, 2014 at 6:22 PM, <mattn.jp@gmail.com> wrote:

> Do you mean that breaking compatibility of second argument?
>
> https://codereview.appspot.com/48870045/
>

[mattn, 2014-01-15 04:29:10]

On 2014/01/15 03:01:03, bradfitz wrote:
> No, I meant that it's a data race: one goroutine is writing to memory that
> another goroutine can be reading, without any synchronization used by both
> of them.

Done. Added test

[bradfitz, 2014-01-15 04:36:52]

On Tue, Jan 14, 2014 at 8:29 PM, <mattn.jp@gmail.com> wrote:

> On 2014/01/15 03:01:03, bradfitz wrote:
>
>> No, I meant that it's a data race: one goroutine is writing to memory
>>
> that
>
>> another goroutine can be reading, without any synchronization used by
>>
> both
>
>> of them.
>>
>
> Done. Added test
>

No, this whole approach is wrong. We can't mutate Transport configuration
parameters.

I thought the problem was too many calls to os.Getenv on Windows?  I would
expect a patch to be near that code.

[mattn, 2014-01-15 07:38:32]

> No, this whole approach is wrong. We can't mutate Transport configuration
> parameters.
> 
> I thought the problem was too many calls to os.Getenv on Windows?  I would
> expect a patch to be near that code.

How about this?

--------
func ProxyFromEnvironmentFixed() func(*Request) (*url.URL, error) {
	http_proxy := getenvEitherCase("HTTP_PROXY")
	if http_proxy == "" {
	}
	no_proxy := getenvEitherCase("NO_PROXY")
	return func(req *Request) (*url.URL, error) {
		if http_proxy == "" {
			return nil, nil
		}
		if no_proxy != "" && !useProxy(no_proxy, canonicalAddr(req.URL)) {
			return nil, nil
		}
		proxyURL, err := url.Parse(http_proxy)
		if err != nil || !strings.HasPrefix(proxyURL.Scheme, "http") {
			// proxy was bogus. Try prepending "http://" to it and
			// see if that parses correctly. If not, we fall
			// through and complain about the original one.
			if proxyURL, err := url.Parse("http://" + http_proxy); err == nil {
				return proxyURL, nil
			}
		}
		if err != nil {
			return nil, fmt.Errorf("invalid proxy address %q: %v", http_proxy, err)
		}
		return proxyURL, nil
	}
}


var transportProxy = &Transport{Proxy: ProxyFromEnvironment}
var transportFixedProxy = &Transport{Proxy: ProxyFromEnvironmentFixed()}
--------

[dvyukov, 2014-01-15 07:45:56]

On 2014/01/15 07:38:32, mattn wrote:
> > No, this whole approach is wrong. We can't mutate Transport configuration
> > parameters.
> > 
> > I thought the problem was too many calls to os.Getenv on Windows?  I would
> > expect a patch to be near that code.
> 
> How about this?
> 
> --------
> func ProxyFromEnvironmentFixed() func(*Request) (*url.URL, error) {
> 	http_proxy := getenvEitherCase("HTTP_PROXY")
> 	if http_proxy == "" {
> 	}
> 	no_proxy := getenvEitherCase("NO_PROXY")
> 	return func(req *Request) (*url.URL, error) {
> 		if http_proxy == "" {
> 			return nil, nil
> 		}
> 		if no_proxy != "" && !useProxy(no_proxy, canonicalAddr(req.URL)) {
> 			return nil, nil
> 		}
> 		proxyURL, err := url.Parse(http_proxy)
> 		if err != nil || !strings.HasPrefix(proxyURL.Scheme, "http") {
> 			// proxy was bogus. Try prepending "http://" to it and
> 			// see if that parses correctly. If not, we fall
> 			// through and complain about the original one.
> 			if proxyURL, err := url.Parse("http://" + http_proxy); err == nil {
> 				return proxyURL, nil
> 			}
> 		}
> 		if err != nil {
> 			return nil, fmt.Errorf("invalid proxy address %q: %v", http_proxy, err)
> 		}
> 		return proxyURL, nil
> 	}
> }
> 
> 
> var transportProxy = &Transport{Proxy: ProxyFromEnvironment}
> var transportFixedProxy = &Transport{Proxy: ProxyFromEnvironmentFixed()}
> --------



It still queries getenvEitherCase("HTTP_PROXY") on every request.
I would expect something along the lines of using sync.Once to query the env
once.

[mattn, 2014-01-15 07:50:00]

On 2014/01/15 07:45:56, dvyukov wrote:
> On 2014/01/15 07:38:32, mattn wrote:
> > > No, this whole approach is wrong. We can't mutate Transport configuration
> > > parameters.
> > > 
> > > I thought the problem was too many calls to os.Getenv on Windows?  I would
> > > expect a patch to be near that code.
> > 
> > How about this?
> > 
> > --------
> > func ProxyFromEnvironmentFixed() func(*Request) (*url.URL, error) {
> > 	http_proxy := getenvEitherCase("HTTP_PROXY")
> > 	if http_proxy == "" {
> > 	}
> > 	no_proxy := getenvEitherCase("NO_PROXY")
> > 	return func(req *Request) (*url.URL, error) {
> > 		if http_proxy == "" {
> > 			return nil, nil
> > 		}
> > 		if no_proxy != "" && !useProxy(no_proxy, canonicalAddr(req.URL)) {
> > 			return nil, nil
> > 		}
> > 		proxyURL, err := url.Parse(http_proxy)
> > 		if err != nil || !strings.HasPrefix(proxyURL.Scheme, "http") {
> > 			// proxy was bogus. Try prepending "http://" to it and
> > 			// see if that parses correctly. If not, we fall
> > 			// through and complain about the original one.
> > 			if proxyURL, err := url.Parse("http://" + http_proxy); err == nil {
> > 				return proxyURL, nil
> > 			}
> > 		}
> > 		if err != nil {
> > 			return nil, fmt.Errorf("invalid proxy address %q: %v", http_proxy, err)
> > 		}
> > 		return proxyURL, nil
> > 	}
> > }
> > 
> > 
> > var transportProxy = &Transport{Proxy: ProxyFromEnvironment}
> > var transportFixedProxy = &Transport{Proxy: ProxyFromEnvironmentFixed()}
> > --------
> 
> 
> 
> It still queries getenvEitherCase("HTTP_PROXY") on every request.

Really? 

> > var transportProxy = &Transport{Proxy: ProxyFromEnvironment}
> > var transportFixedProxy = &Transport{Proxy: ProxyFromEnvironmentFixed()}

ProxyFromEnvironmentFixed generate a function to work as ProxyFromEnvironment.

> I would expect something along the lines of using sync.Once to query the env
> once.

In my understanding, HTTP_PROXY should be given in every time for creating new
http-client not every-request.
So we must give the chance to refresh HTTP_PROXY/NO_PROXY.

[dvyukov, 2014-01-15 08:10:27]

On 2014/01/15 07:50:00, mattn wrote:
> On 2014/01/15 07:45:56, dvyukov wrote:
> > On 2014/01/15 07:38:32, mattn wrote:
> > > > No, this whole approach is wrong. We can't mutate Transport
configuration
> > > > parameters.
> > > > 
> > > > I thought the problem was too many calls to os.Getenv on Windows?  I
would
> > > > expect a patch to be near that code.
> > > 
> > > How about this?
> > > 
> > > --------
> > > func ProxyFromEnvironmentFixed() func(*Request) (*url.URL, error) {
> > > 	http_proxy := getenvEitherCase("HTTP_PROXY")
> > > 	if http_proxy == "" {
> > > 	}
> > > 	no_proxy := getenvEitherCase("NO_PROXY")
> > > 	return func(req *Request) (*url.URL, error) {
> > > 		if http_proxy == "" {
> > > 			return nil, nil
> > > 		}
> > > 		if no_proxy != "" && !useProxy(no_proxy, canonicalAddr(req.URL)) {
> > > 			return nil, nil
> > > 		}
> > > 		proxyURL, err := url.Parse(http_proxy)
> > > 		if err != nil || !strings.HasPrefix(proxyURL.Scheme, "http") {
> > > 			// proxy was bogus. Try prepending "http://" to it and
> > > 			// see if that parses correctly. If not, we fall
> > > 			// through and complain about the original one.
> > > 			if proxyURL, err := url.Parse("http://" + http_proxy); err == nil {
> > > 				return proxyURL, nil
> > > 			}
> > > 		}
> > > 		if err != nil {
> > > 			return nil, fmt.Errorf("invalid proxy address %q: %v", http_proxy, err)
> > > 		}
> > > 		return proxyURL, nil
> > > 	}
> > > }
> > > 
> > > 
> > > var transportProxy = &Transport{Proxy: ProxyFromEnvironment}
> > > var transportFixedProxy = &Transport{Proxy: ProxyFromEnvironmentFixed()}
> > > --------
> > 
> > 
> > 
> > It still queries getenvEitherCase("HTTP_PROXY") on every request.
> 
> Really? 

Ah, I see, it captures the strings. Then leaving to Brad.

> > > var transportProxy = &Transport{Proxy: ProxyFromEnvironment}
> > > var transportFixedProxy = &Transport{Proxy: ProxyFromEnvironmentFixed()}
> 
> ProxyFromEnvironmentFixed generate a function to work as ProxyFromEnvironment.
> 
> > I would expect something along the lines of using sync.Once to query the env
> > once.
> 
> In my understanding, HTTP_PROXY should be given in every time for creating new
> http-client not every-request.
> So we must give the chance to refresh HTTP_PROXY/NO_PROXY.

[bradfitz, 2014-01-15 19:25:08]

No, we're not going to mutate Transport.Proxy.

[bradfitz, 2014-01-15 21:27:32]

See counter-proposal: https://codereview.appspot.com/52840043


On Tue, Jan 7, 2014 at 10:02 PM, <mattn.jp@gmail.com> wrote:

> Reviewers: golang-codereviews,
>
> Message:
> Hello golang-codereviews@googlegroups.com,
>
> I'd like you to review this change to
> http://go.googlecode.com/hg/
>
>
> Description:
> net/http: Remove proxy detector for the transport
> Fixes issue 7020
>
> Please review this at https://codereview.appspot.com/48870045/
>
> Affected files (+8, -2 lines):
>   M src/pkg/net/http/transport.go
>
>
> Index: src/pkg/net/http/transport.go
> ===================================================================
> --- a/src/pkg/net/http/transport.go
> +++ b/src/pkg/net/http/transport.go
> @@ -92,6 +92,8 @@
>         // TODO: tunable on timeout on cached connections
>  }
>
> +var noProxy = errors.New("NO_PROXY matches")
> +
>  // ProxyFromEnvironment returns the URL of the proxy to use for a
>  // given request, as indicated by the environment variables
>  // $HTTP_PROXY and $NO_PROXY (or $http_proxy and $no_proxy).
> @@ -104,7 +106,7 @@
>                 return nil, nil
>         }
>         if !useProxy(canonicalAddr(req.URL)) {
> -               return nil, nil
> +               return nil, noProxy
>         }
>         proxyURL, err := url.Parse(proxy)
>         if err != nil || !strings.HasPrefix(proxyURL.Scheme, "http") {
> @@ -259,7 +261,11 @@
>                 var err error
>                 cm.proxyURL, err = t.Proxy(treq.Request)
>                 if err != nil {
> -                       return nil, err
> +                       if err != noProxy {
> +                               return nil, err
> +                       }
> +               } else if cm.proxyURL == nil {
> +                       t.Proxy = nil
>                 }
>         }
>         return cm, nil
>
>
> --
> You received this message because you are subscribed to the Google Groups
> "golang-codereviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-codereviews+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/groups/opt_out.
>

[bradfitz, 2014-01-17 19:14:09]

R=close