code review 4070043: http: added virtual host mappings BEFORE default path m...

http: added virtual host mappings BEFORE default path mappings
+ reply to fixes asked by rsc at patch6

See http://groups.google.com/group/golang-nuts/browse_thread/thread/872b2c6feb556e62
and http://codereview.appspot.com/4070043

[bradfitz, 2011-01-25 17:57:22]

http://codereview.appspot.com/4070043/diff/1/src/pkg/http/server.go
File src/pkg/http/server.go (right):

http://codereview.appspot.com/4070043/diff/1/src/pkg/http/server.go#newcode533
src/pkg/http/server.go:533: // Patterns must begin with /, excepfor virtual
hosts paths explained later.
typo

http://codereview.appspot.com/4070043/diff/1/src/pkg/http/server.go#newcode541
src/pkg/http/server.go:541: // For VirtualHost paths are path with a host-name
at the beginning
missing word or punctuation?

http://codereview.appspot.com/4070043/diff/1/src/pkg/http/server.go#newcode623
src/pkg/http/server.go:623: h=findHandler(mux.m,r.URL.Path)
you should run this through gofmt.  This style is wrong.

[rsc, 2011-01-25 18:01:21]

please run hg mail to send in code reviews.
that will take care of things like making sure
you've run gofmt.

please sort your imports.

can still use just one map.

russ

[josvazg, 2011-01-25 22:49:55]

Ok
1 - fix comments
2 - run gofmt
3 - run hg mail
4 - rewrite for one map
5 - add a cache (on a separate map)?

I will have to do it tomorrow at work. (How can I download my patch to another
computer with a copy of golang)

[bradfitzgoog, 2011-01-26 00:04:39]

On Tue, Jan 25, 2011 at 2:49 PM, <josvazg@gmail.com> wrote:

>
> I will have to do it tomorrow at work. (How can I download my patch to
> another computer with a copy of golang)
>

$ hg help clpatch

[josvazg, 2011-01-26 15:13:18]

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

I'd like you to review this change.

[rsc, 2011-01-26 15:29:37]

http://codereview.appspot.com/4070043/diff/4002/src/pkg/http/serve_test.go
File src/pkg/http/serve_test.go (right):

http://codereview.appspot.com/4070043/diff/4002/src/pkg/http/serve_test.go#ne...
src/pkg/http/serve_test.go:10: "bytes"
sort imports

http://codereview.appspot.com/4070043/diff/4002/src/pkg/http/serve_test.go#ne...
src/pkg/http/serve_test.go:140: rw.Write(([]byte)("Default"))
the () around []byte are unnecessary.
same in various places below
rw.Flush is also unnecessary.
ResponseWriter argument is usually named w.

http://codereview.appspot.com/4070043/diff/4002/src/pkg/http/serve_test.go#ne...
src/pkg/http/serve_test.go:154: func GetVHost(url, host string) (r *Response,
finalURL string, err os.Error) {
This is a copy of http.Get but I don't see what's different.
Please comment what's different (or fix Get if there's a bug).

http://codereview.appspot.com/4070043/diff/4002/src/pkg/http/serve_test.go#ne...
src/pkg/http/serve_test.go:200: s := (string)(msg)
() are unnecessary around string

http://codereview.appspot.com/4070043/diff/4002/src/pkg/http/serve_test.go#ne...
src/pkg/http/serve_test.go:211: checkResponse(t,
"http://localhost:12345/someDir/apage", "", "someDir")
please use a table.
see url_test.go, for example.

var virtualTests = []struct {
    url string
    host string
    data string
}{
    {"http://localhost:12345/someDir/apage", "", "someDir"},
    ...
}

http://codereview.appspot.com/4070043/diff/4002/src/pkg/http/server.go
File src/pkg/http/server.go (right):

http://codereview.appspot.com/4070043/diff/4002/src/pkg/http/server.go#newcod...
src/pkg/http/server.go:533: // Patterns must begin with /, except for virtual
hosts paths explained later.
delete
change previous two lines to
// Patterns named fixed, rooted paths, like "/favicon.ico",
// or rooted subtrees, like "/images/" (note the trailing slash).

http://codereview.appspot.com/4070043/diff/4002/src/pkg/http/server.go#newcod...
src/pkg/http/server.go:541: // Virtual host paths are also supported. They start
with a host-name at the
// Patterns may optionally begin with a host name, restricting matches to
// URLs on that host only.  Host-specific patterns take precedence over
// general patterns, so that a handler might register for the two patterns
// "/codesearch" and "codesearch.google.com/" without also taking over
// requests for "http://www.google.com/".

http://codereview.appspot.com/4070043/diff/4002/src/pkg/http/server.go#newcod...
src/pkg/http/server.go:604: // Cached?
This cache is going to grow without bound, and we have no data
showing that it is necessary anyway.  Please remove.

In your original CL you had a function like

func (mux *ServeMux) match(path string) Handler {
    ... original matching code (lines 601-611 of original code) ...
}

If you restore that then this becomes

h := mux.match(r.Host + r.URL.Path)
if h == nil {
    h = mux.match(r.URL.Path)
}
if h == nil {
    h = NotFoundHandler()
}
h.ServeHTTP(w, r)

[josvazg, 2011-01-26 16:31:50]

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

Please take another look.

[josvazg, 2011-01-26 16:35:41]

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

Please take another look.

[rsc, 2011-01-26 16:54:25]

the code looks good.
the test still looks a bit like a Java test
instead of a Go test.  some suggestions below.

please change the CL description to

http: implement host patterns

short and sweet

http://codereview.appspot.com/4070043/diff/17001/src/pkg/http/serve_test.go
File src/pkg/http/serve_test.go (right):

http://codereview.appspot.com/4070043/diff/17001/src/pkg/http/serve_test.go#n...
src/pkg/http/serve_test.go:139: func defaultHandler(w ResponseWriter, r
*Request) {
kind of repetitive.  here's an alternative

type stringHandler string

func (s stringHandler) ServeHTTP(w ResponseWriter, r *Request) {
    w.Write([]byte(s))
}

Handle("/", stringHandler("Default"))
Handle("/someDir/", stringHandler("someDir"))
etc

and then the Handle calls should be table-driven too.

http://codereview.appspot.com/4070043/diff/17001/src/pkg/http/serve_test.go#n...
src/pkg/http/serve_test.go:157: func GetVHost(url, host string) (r *Response,
finalURL string, err os.Error) {
This is exploting what amounts to a bug in send: 
it doesn't use the same information to choose where
to dial as Request does to choose what Host: line to include.

A better choice would be to use a ClientConn.
In the test you can set up with

    conn, err := net.Dial("tcp", "", "localhost:12345")
    if err != nil {
        return nil, err
    }
    defer conn.Close()
    cc := NewClientConn(conn, nil)

and then each iteration would do:

    var req Request
    if req.URL, err = ParseURL(url); err != nil {
        t.Errorf("cannot parse url: %v", err)
        continue
    }
    if err := cc.Write(&Request{URL: url}); err != nil {
        t.Errorf("writing request: %v", err)
        continue
    }
    resp, err := cc.Read()
    if err != nil {
        t.Errorf("reading response: %v", err)
        continue
    }

http://codereview.appspot.com/4070043/diff/17001/src/pkg/http/serve_test.go#n...
src/pkg/http/serve_test.go:215: var vtests = []struct {
move to global, just above this Test func.

var virtualHostTests = ...

http://codereview.appspot.com/4070043/diff/17001/src/pkg/http/serve_test.go#n...
src/pkg/http/serve_test.go:227: for i := 0; i < len(vtests); i++ {
for _, tt := range virtualHostTests {

http://codereview.appspot.com/4070043/diff/17001/src/pkg/http/serve_test.go#n...
src/pkg/http/serve_test.go:228: checkResponse(t, vtests[i].url, vtests[i].host,
vtests[i].data)
now that there's only one call, inline the code and delete checkResponse.

http://codereview.appspot.com/4070043/diff/17001/src/pkg/http/server.go
File src/pkg/http/server.go (right):

http://codereview.appspot.com/4070043/diff/17001/src/pkg/http/server.go#newco...
src/pkg/http/server.go:531: // Patterns named fixed, rooted paths, like
"/favicon.ico",
s/named/name/ (error in original text, but please fix)

http://codereview.appspot.com/4070043/diff/17001/src/pkg/http/server.go#newco...
src/pkg/http/server.go:533: // Patterns must begin with /, except for virtual
hosts paths explained later.
please delete this line

http://codereview.appspot.com/4070043/diff/17001/src/pkg/http/server.go#newco...
src/pkg/http/server.go:616: // Host-specific patterns first, then most-specific
(longest) pattern wins.
// Host-specific pattern takes precedence over general patterns.

mentioning "most-specific (longest)" is confusing,
because the code that does that is not here.

[josvazg, 2011-01-26 20:07:44]

Can't seem to work at home, hg clpatch seems not supported by my home hg version
(version 1.4.3):

hg help clpatch
*** failed to import extension codereview from
/home/jose/lib/codereview/codereview.py: [Errno 2] No such file or directory
hg: unknown command 'clpatch'
Mercurial Distributed SCM

basic commands:
...

---
Also can't compile with ./all.bash, the compilation hangs after the net package:
...
gopack grc _test/net.a _gotest_.6 
make[2]: Leaving directory `/home/jose/go/src/pkg/net'

(hangs forever)

[rsc, 2011-01-26 20:25:13]

On Wed, Jan 26, 2011 at 15:07,  <josvazg@gmail.com> wrote:
> Can't seem to work at home, hg clpatch seems not supported by my home hg
> version (version 1.4.3):

see http://golang.org/doc/contribute.html.
you need to configure the mercurial extension
on your home machine the same way you
did on your other machine.

---
Edit $GOROOT/.hg/hgrc to add:

[extensions]
codereview = YOUR_GO_ROOT/lib/codereview/codereview.py

[ui]
username = Your Name <you@server.dom>

Replace YOUR_GO_ROOT with the value of $GOROOT. The Mercurial
configuration file format does not allow environment variable
substitution. The username information will not be used unless you are
a committer (see below), but Mercurial complains if it is missing.
--

> gopack grc _test/net.a _gotest_.6
> make[2]: Leaving directory `/home/jose/go/src/pkg/net'
>
> (hangs forever)

this is probably a networking problem.
are you on the internet?
are you on a Mac?

russ

[josvazg, 2011-01-26 21:02:22]

Yes I already tried that:
$ cat .hg/hgrc 
[paths]
default = https://go.googlecode.com/hg/

[extensions]
codereview = /home/jose/lib/codereview/codereview.py

[ui]
username = Jose Luis Vázquez González <josvazg@gmail.com>

Do you see something wrong on the file?

And yes, the all.bash problem was a networking problem:

gopack grc _test/net.a _gotest_.6 
make[2]: Leaving directory `/home/jose/go/src/pkg/net'
--- FAIL: net.TestDialGoogle
...
	Dial("tcp6", "", "74.125.19.99:http") = _, dial tcp6 74.125.19.99:80:
connection timed out
...
connection timed out
FAIL

I am on ubuntu 10.04 (same at work) and I have internet access, (I am typing
this on codereview.appspot.com), but maybe there is (temporary) some problem in
Spain ISP networks to access those IPs.

[rsc, 2011-01-26 21:23:02]

On Wed, Jan 26, 2011 at 16:02,  <josvazg@gmail.com> wrote:
> Yes I already tried that:
> $ cat .hg/hgrc
> [paths]
> default = https://go.googlecode.com/hg/
>
> [extensions]
> codereview = /home/jose/lib/codereview/codereview.py

does this file exist?
i would have expected it to say something
like /home/jose/go/lib/codereview/codereview.py
unless your $GOROOT is /home/jose.

>        Dial("tcp6", "", "74.125.19.99:http") = _, dial tcp6 74.125.19.99:80:
> connection timed out

hg sync should fix this for you.
i pushed out a fix to this bug earlier today.
sorry about the trouble.

russ

[josvazg, 2011-01-26 21:40:33]

Thanks a lot!

- The path for codereview.py was wrong, I didn't notice.

- The compilation was successful now after an hg sync.

[josvazg, 2011-01-26 21:41:24]

Why do I get this error?:
$ hg clpatch 4070043
warning: cannot find josvazg@gmail.com in CONTRIBUTORS
hgpatch: exceptions.OSError: [Errno 2] No such file or directory

[rsc, 2011-01-26 21:45:54]

On Wed, Jan 26, 2011 at 16:41,  <josvazg@gmail.com> wrote:
> Why do I get this error?:
> $ hg clpatch 4070043
> warning: cannot find josvazg@gmail.com in CONTRIBUTORS

This is okay.  (clpatch is usually for people who are
submitting a change for someone, and it's a reminder to us.)

> hgpatch: exceptions.OSError: [Errno 2] No such file or directory

Sounds like you have not run all.bash
or if you did have not set your $PATH to
include the bin directory where binaries
got installed ($GOBIN or if not set $GOROOT/bin).

hgpatch is the name of a Go program installed
by all.bash, and this message is saying that
the Python plugin cannot find it to execute it.
It is not a very good error message.

Russ

[josvazg, 2011-01-27 15:50:53]

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

Please take another look.

[rsc, 2011-01-27 16:19:54]

http://codereview.appspot.com/4070043/diff/1003/src/pkg/http/serve_test.go
File src/pkg/http/serve_test.go (right):

http://codereview.appspot.com/4070043/diff/1003/src/pkg/http/serve_test.go#ne...
src/pkg/http/serve_test.go:204: func TestVirtualHostHandlers(t *testing.T) {
TestHostPatterns

i know that other servers call these "virtual hosts"
but they are no more virtual than any other host.

http://codereview.appspot.com/4070043/diff/1003/src/pkg/http/serve_test.go#ne...
src/pkg/http/serve_test.go:210: checkResponse(t, vt.url, vt.expected)
please inline this call.
there is only one call in the entire file
and it is easier to see the context if the
code is here instead of somewhere else.

if you do that you can also inline the getFakedVirtHost
and lift the connection management out of the loop
so that you only dial once.

see my last round of suggestions for sample code.

[josvazg, 2011-01-27 17:43:07]

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

Please take another look.

[rsc, 2011-01-27 17:55:38]

Very close.  Looks like a Go test now.  :-)

http://codereview.appspot.com/4070043/diff/33001/src/pkg/http/serve_test.go
File src/pkg/http/serve_test.go (right):

http://codereview.appspot.com/4070043/diff/33001/src/pkg/http/serve_test.go#n...
src/pkg/http/serve_test.go:149: {"someHost.com:12345/someDir/",
"someHost.com:12345/someDir"},
I'd suggest dropping :12345 here and in the tests.
It's unnecessary.

http://codereview.appspot.com/4070043/diff/33001/src/pkg/http/serve_test.go#n...
src/pkg/http/serve_test.go:167: go ListenAndServe(":12345", nil)
This creates a race - the goroutine might not listen
before the Dial tries to connect - and also hard-codes
a port, which is undesirable.

Instead, I think you need to do:

l, err := net.Listen("tcp", "127.0.0.1:0")  // any port
if err != nil {
    t.Fatal(err)
}
defer l.Close()
go http.Serve(l, nil)

conn, err := net.Dial("tcp", "", l.Addr().String())
if err != nil {
    t.Fatal(err)
}


(Fatal is like Error but aborts the test, so you don't need the return.)

http://codereview.appspot.com/4070043/diff/33001/src/pkg/http/serve_test.go#n...
src/pkg/http/serve_test.go:192: if s == "" {
this test isn't necessary given s != vt.expected below
delete

http://codereview.appspot.com/4070043/diff/33001/src/pkg/http/serve_test.go#n...
src/pkg/http/serve_test.go:197: t.Errorf("Get Result header is unexpected: %v
instead of %v\n", s, r)
%q will print quoted strings, which is useful here.
also, usually clearer and shorter to print code:

t.Errorf("Get(%q) = %q, want %q", vt.url, s, vt.expected)

[josvazg, 2011-01-28 10:38:48]

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

Please take another look.

[josvazg, 2011-01-28 10:43:26]

Couldn't work at home again.
I fixed my GO building environment so that ./all.bash and hg clpatch could find
all GO binaries in the PATH and that worked.

hg clpatch forced me to do hg sync before and then hg clpatch tried to pacth
serve_test.go, but it failed with an error. Maybe the new version of
serve_test.go already contains changes incompatible with my patch?
At work everything works ok, but I haven't done any hg sync or hg pull.

[rsc, 2011-01-28 13:07:28]

On Fri, Jan 28, 2011 at 05:43,  <josvazg@gmail.com> wrote:
> Couldn't work at home again.
> I fixed my GO building environment so that ./all.bash and hg clpatch
> could find all GO binaries in the PATH and that worked.
>
> hg clpatch forced me to do hg sync before and then hg clpatch tried to
> pacth serve_test.go, but it failed with an error. Maybe the new version
> of serve_test.go already contains changes incompatible with my patch?
> At work everything works ok, but I haven't done any hg sync or hg pull.

Sorry about that.  clpatch is not really meant for moving
work back and forth between computers, so it doesn't handle
these kinds of cases terribly well.  It's really meant for submitters
to patch in CLs in order to submit them (I will run clpatch to
submit this CL when it is done, for example).
I switch between computers by copying the entire Go tree
($GOROOT) from machine to machine.

Russ

[rsc, 2011-01-28 13:11:01]

LGTM

Thanks for seeing this through.

Please complete a CLA as described at
http://golang.org/doc/contribute.html#copyright
so that we can add the changes to the repository.

Russ

[josvazg, 2011-01-29 10:57:51]

Done. I guess I should be receiving an answer from Google by post mail?

Do you think this change will be included in the next go releases?
Thanks for your help!

[rsc, 2011-01-30 17:16:18]

On Sat, Jan 29, 2011 at 05:57,  <josvazg@gmail.com> wrote:
> Done. I guess I should be receiving an answer from Google by post mail?

I don't think so.  It just ends up recorded in our open source records,
which we check before adding someone to the AUTHORS and CONTRIBUTORS
files.  I see your form submission in the records.

> Do you think this change will be included in the next go releases?

It will.  Thanks again.

Russ

[rsc, 2011-02-01 18:59:02]

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

http: add host patterns

R=bradfitzgo, rsc
CC=golang-dev
http://codereview.appspot.com/4070043

Committer: Russ Cox <rsc@golang.org>