code review 4043043: doc/codelab/wiki: tests use available TCP port

doc/codelab/wiki: tests use available TCP port

[adg, 2011-01-27 00:51:47]

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

I'd like you to review this change.

[dsymonds, 2011-01-27 00:58:58]

FYI

http://codereview.appspot.com/4043043/diff/3001/doc/codelab/wiki/get.go
File doc/codelab/wiki/get.go (right):

http://codereview.appspot.com/4043043/diff/3001/doc/codelab/wiki/get.go#newco...
doc/codelab/wiki/get.go:22: l, err := net.Listen("tcp", "127.0.0.1:0")
This is an unnecessary dependency on IPv4.
Just ":0" should work fine, I think.

[r2, 2011-01-27 01:00:46]

On Jan 26, 2011, at 4:58 PM, dsymonds@golang.org wrote:

> FYI
> 
> 
> http://codereview.appspot.com/4043043/diff/3001/doc/codelab/wiki/get.go
> File doc/codelab/wiki/get.go (right):
> 
>
http://codereview.appspot.com/4043043/diff/3001/doc/codelab/wiki/get.go#newco...
> doc/codelab/wiki/get.go:22: l, err := net.Listen("tcp", "127.0.0.1:0")
> This is an unnecessary dependency on IPv4.
> Just ":0" should work fine, I think.

i wish someone would give a definitive answer (as if i believe there is one).
alternate CLs put in 127.0.0.1 and take it out again.

-rob

[dangabrad, 2011-01-27 01:04:49]

I assume you don't care that this is racy?

What I did in my project (which might be a little overkill) is pass fd
numbers in the environment to no-closeonexec pipes creates by the controller
parent and inherited by the subprocess webserver which listens on :0 and
writes back on the pipe (to the parent controller process) which port it
got.

On Wed, Jan 26, 2011 at 4:51 PM, <adg@golang.org> wrote:

> Reviewers: bradfitz,
>
> Message:
> Hello bradfitz (cc: golang-dev@googlegroups.com),
>
> I'd like you to review this change.
>
>
> Description:
> doc/codelab/wiki: tests use available TCP port
>
> Please review this at http://codereview.appspot.com/4043043/
>
> Affected files:
>  M doc/codelab/wiki/Makefile
>  M doc/codelab/wiki/get.go
>  M doc/codelab/wiki/test.sh
>
>
> Index: doc/codelab/wiki/Makefile
> ===================================================================
> --- a/doc/codelab/wiki/Makefile
> +++ b/doc/codelab/wiki/Makefile
> @@ -13,9 +13,9 @@
>  index.html: srcextract.bin htmlify.bin
>        awk '/^!/{system(substr($$0,2)); next} {print}' "$$@" < wiki.html >
> index.html
>
> -test: final.bin get.bin
> +test: get.bin
>        bash ./test.sh
> -       rm -f final.6 final.bin get.6 get.bin
> +       rm -f get.6 get.bin
>
>  %.bin: %.$O
>        $(LD) -o $@ $<
> Index: doc/codelab/wiki/get.go
> ===================================================================
> --- a/doc/codelab/wiki/get.go
> +++ b/doc/codelab/wiki/get.go
> @@ -3,16 +3,30 @@
>  import (
>        "http"
>        "flag"
> +       "fmt"
>        "io"
>        "log"
> +       "net"
>        "os"
>        "strings"
>  )
>
> -var post = flag.String("post", "", "urlencoded form data to POST")
> +var (
> +       post = flag.String("post", "", "urlencoded form data to POST")
> +       port = flag.Bool("port", false, "find open port and print to
> stdout")
> +)
>
>  func main() {
>        flag.Parse()
> +       if *port {
> +               l, err := net.Listen("tcp", "127.0.0.1:0")
> +               if err != nil {
> +                       log.Exit(err)
> +               }
> +               defer l.Close()
> +               fmt.Print(l.Addr().(*net.TCPAddr).Port)
> +               return
> +       }
>        url := flag.Arg(0)
>        if url == "" {
>                log.Exit("no url supplied")
> Index: doc/codelab/wiki/test.sh
> ===================================================================
> --- a/doc/codelab/wiki/test.sh
> +++ b/doc/codelab/wiki/test.sh
> @@ -1,22 +1,27 @@
>  #!/bin/bash
>
> -./final.bin &
> -wiki_pid=$!
> +wiki_pid=
>
>  cleanup() {
>        kill $wiki_pid
> -       rm -f test_*.out Test.txt
> +       rm -f test_*.out Test.txt final-test.bin final-test.go
>        exit ${1:-1}
>  }
>  trap cleanup INT
>
> +port=$(./get.bin -port)
> +sed s/8080/$port/ < final.go > final-test.go
> +gomake final-test.bin || cleanup 1
> +./final-test.bin &
> +wiki_pid=$!
> +
>  sleep 1
>
> -./get.bin http://localhost:8080/edit/Test > test_edit.out
> +./get.bin http://localhost:$port/edit/Test > test_edit.out
>  diff -u test_edit.out test_edit.good || cleanup 1
> -./get.bin -post=body=some%20content http://localhost:8080/save/Test
> +./get.bin -post=body=some%20content http://localhost:$port/save/Test
>  diff -u Test.txt test_Test.txt.good || cleanup 1
> -./get.bin http://localhost:8080/view/Test > test_view.out
> +./get.bin http://localhost:$port/view/Test > test_view.out
>  diff -u test_view.out test_view.good || cleanup 1
>
>  echo "Passed"
>
>
>

[adg, 2011-01-27 01:36:51]

I want 127.0.0.1 because I want to find a port to listen and connect
to on loopback.

On 27 January 2011 10:58,  <dsymonds@golang.org> wrote:
> FYI
>
>
> http://codereview.appspot.com/4043043/diff/3001/doc/codelab/wiki/get.go
> File doc/codelab/wiki/get.go (right):
>
>
http://codereview.appspot.com/4043043/diff/3001/doc/codelab/wiki/get.go#newco...
> doc/codelab/wiki/get.go:22: l, err := net.Listen("tcp", "127.0.0.1:0")
> This is an unnecessary dependency on IPv4.
> Just ":0" should work fine, I think.
>
> http://codereview.appspot.com/4043043/
>

[adg, 2011-01-27 01:39:58]

On 27 January 2011 11:04, Brad Fitzpatrick <brad@danga.com> wrote:
> I assume you don't care that this is racy?

I think it's okay in this case.

> What I did in my project (which might be a little overkill) is pass fd
> numbers in the environment to no-closeonexec pipes creates by the controller
> parent and inherited by the subprocess webserver which listens on :0 and
> writes back on the pipe (to the parent controller process) which port it
> got.

The approach makes sense, but I think it might be overkill in this
case. I don't want to have to modify final.go too much as it is
example code. Swapping in the port number seemed a reasonable
compromise.

I don't like this that much, either, but if anyone has better ideas
I'm keen to hear them.

Andrew

[dsymonds, 2011-01-27 01:57:02]

On Thu, Jan 27, 2011 at 12:36 PM, Andrew Gerrand <adg@golang.org> wrote:

> I want 127.0.0.1 because I want to find a port to listen and connect
> to on loopback.

Yes, ":0" should give you an address that works fine, and will also
work with IPv6.

[rsc, 2011-01-27 03:21:00]

>> I want 127.0.0.1 because I want to find a port to listen and connect
>> to on loopback.
>
> Yes, ":0" should give you an address that works fine, and will also
> work with IPv6.

Please use "127.0.0.1:0", not ":0".

The former means listen only on the address "127.0.0.1" (and pick a port)
while the latter means listen on all available addresses (and pick a port).
The difference is that using ":0" makes the service accessible from
other machines on the network; using "127.0.0.1:0" does not.
This matters with the OS X firewall, which by default rejects attempts
by programs to listen on external addresses but allows attempts
to listen on the loopback interface.

David is right that on systems without an IPv4 loopback, using
"127.0.0.1:0" will fail.  That's not a new failure - every network server
test in the Go tree uses that address - so it's not worth worrying
about one more.  Not having an IPv4 loopback is a mostly hypothetical
problem.  Triggering the OS X firewall is a real one.

Russ

[dsymonds, 2011-01-27 03:25:43]

On Thu, Jan 27, 2011 at 2:20 PM, Russ Cox <rsc@golang.org> wrote:

> David is right that on systems without an IPv4 loopback, using
> "127.0.0.1:0" will fail.  That's not a new failure - every network server
> test in the Go tree uses that address - so it's not worth worrying
> about one more.  Not having an IPv4 loopback is a mostly hypothetical
> problem.  Triggering the OS X firewall is a real one.

Okay, that's fair enough.

I'm raising this issue because I've seen systems that resolve
"localhost" to the IPv6 loopback only rather than the IPv4 loopback
(or both loopbacks), which means that code that binds to 127.0.0.1:0
fails to be contactable on "localhost:<port>". I guess it's worse that
the OS X firewall is quite touchy here.

[rsc, 2011-01-27 03:53:54]

> I'm raising this issue because I've seen systems that resolve
> "localhost" to the IPv6 loopback only rather than the IPv4 loopback
> (or both loopbacks), which means that code that binds to 127.0.0.1:0
> fails to be contactable on "localhost:<port>". I guess it's worse that
> the OS X firewall is quite touchy here.

Right, so an alternative to using 127.0.0.1:0 everywhere
would be to use localhost:0 everywhere.  I don't think there
is a compelling reason to switch, and the former is what
we're already doing.  (And actually I suspect there are more
systems with a broken definition of localhost than there are
without IPv4 loopback.)

Russ

[adg, 2011-01-27 12:38:07]

So can I get an LGTM?

(Note that I replaced localhost with 127.0.0.1 before this discussion
began, but it's only visible in codereview, not the original mail.)

On 27 January 2011 13:20, Russ Cox <rsc@golang.org> wrote:
>>> I want 127.0.0.1 because I want to find a port to listen and connect
>>> to on loopback.
>>
>> Yes, ":0" should give you an address that works fine, and will also
>> work with IPv6.
>
> Please use "127.0.0.1:0", not ":0".
>
> The former means listen only on the address "127.0.0.1" (and pick a port)
> while the latter means listen on all available addresses (and pick a port).
> The difference is that using ":0" makes the service accessible from
> other machines on the network; using "127.0.0.1:0" does not.
> This matters with the OS X firewall, which by default rejects attempts
> by programs to listen on external addresses but allows attempts
> to listen on the loopback interface.
>
> David is right that on systems without an IPv4 loopback, using
> "127.0.0.1:0" will fail.  That's not a new failure - every network server
> test in the Go tree uses that address - so it's not worth worrying
> about one more.  Not having an IPv4 loopback is a mostly hypothetical
> problem.  Triggering the OS X firewall is a real one.
>
> Russ
>

[rsc, 2011-01-27 13:17:59]

LGTM

[adg, 2011-01-27 13:19:41]

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

doc/codelab/wiki: tests use available TCP port

R=bradfitz, dsymonds, r2, dangabrad, rsc
CC=golang-dev
http://codereview.appspot.com/4043043