net/http: document and test behavior of ServeMux with ports

[kellegous]

Please answer these questions before submitting your issue. Thanks!

What version of Go are you using (go version)?

1.9.2 and 1.10beta1

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/knorton/Library/Caches/go-build"
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/knorton/Documents/2018/01/underpants"
GORACE=""
GOROOT="/Users/knorton/Documents/2018/01/underpants/go1.10"
GOTMPDIR=""
GOTOOLDIR="/Users/knorton/Documents/2018/01/underpants/go1.10/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/9k/yc77pqm56jj79pp163xszgn00000gn/T/go-build934488457=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

Consider this Go program:

package main

import (
	"fmt"
	"log"
	"net/http"
)

func main() {
	m := http.NewServeMux()

	m.HandleFunc("localhost:8080/",
		func(w http.ResponseWriter, r *http.Request) {
			fmt.Fprintln(w, "locahost:8080")
		})

	log.Panic(http.ListenAndServe(":8080", m))
}

If you use Go 1.8.5:

1. go run example.go
2. Visit http://localhost:8080/
3. You will see the text "localhost:8080" display in the browser.

If you use Go 1.10beta1 and Go 1.9.2:

1. go run example.go
2. Visit http://localhost:8080/
3. You will get a 404 NotFound.

What did you expect to see?

I expect that compiling the example in 1.9.2 should behave as it did in 1.8.5.

More specifically, I would expect Go 1.9.2 to call the handler that I explicitly added for the host localhost:8080.

What did you see instead?

Instead, this program called with Go 1.9.2 returns a 404 instead of calling the handler that I added for the host localhost:8080.

More Details

This change seems to have been reviewed at: https://go-review.googlesource.com/c/go/+/38194

This change strips the port from the host only when it is trying to locate a handler. It does not strip the port when the user adds a handler for a host that includes a port number. As a result, trying to add handlers for hosts that include the port no longer works.

To make matters worse, these change makes it hard to write http code for hosts with ports that works on both Go 1.9 and Go <1.9.

In Go 1.9, the route has to be setup with m.HandleFunc("localhost", ...)
And in Go < 1.9, the route has to be setup with m.HandleFunc("localhost:8080", ...).

To make the code work on all versions of Go, the handler function needs to be registered for both "localhost" and "localhost:8080"

[bradfitz]

Unfortunately, the time for Go 1.9 bug reports was about 8 months ago during the beta & release candidate periods. If we changed the behavior in Go 1.9, it's difficult to impossible to change again and we might be stuck with the current behavior.

Please try Go 1.10beta1, which is still in beta and maybe easier to maybe change.

/cc @kennygrant

[kellegous]

@bradfitz I'm wondering if there should be checking/normalization code added to Handle that at least registers localhost:8080 as localhost. That would effectively remove the port entirely from routing. That has the downside of preventing us from ever having both localhost:8080 and localhost:9090 in the same mux, but it would make common uses of host routing work as expected.

[bradfitz]

I don't know. I'm barely familiar with the ServeMux code. I've always let other people who care about it more handle it. I'll see if that continues to be the case with this bug.

[bradfitz]

Could you report your results with Go 1.10?

[kellegous]

The behavior in 1.10beta1 is identical to 1.9.2. Sadly, I guess the cat is already out of the bag on this one. Since 1.9.2 went to stable with this, Go Authors will just have to be aware that you have to register both the host:port route and the host routes in a ServeMux to ensure the code works on both <1.9 and 1.9+.

[kellegous]

In case other people stumble on this report. A partial workaround is to make sure the handler is registered twice (one without the port and one with):

package main

import (
	"fmt"
	"log"
	"net/http"
)

func handle(w http.ResponseWriter, r *http.Request) {
	fmt.Fprintln(w, "locahost:8080")
}

func main() {
	m := http.NewServeMux()

	m.HandleFunc("localhost:8080/", handle)
	m.HandleFunc("localhost/", handle)

	log.Panic(http.ListenAndServe(":8080", m))
}

[kennygrant]

Sorry about that.

This change was made in response to this problem - #10463 - specifying ports wasn't working well with some clients and causing a redirect loop. I don't think we currently have any tests for patterns with a port and I didn't anticipate or test this use of port in patterns, since ListenAndServe specifies the port to listen on.

For code which has to support older versions of Go at the same time, I agree it's awkward. For code using > 1.8, this isn't an issue, as it can just use routes without ports.

Possible solutions:

Strip ports from handlers in mux.Handle and make explicit the lack of support for port matching in patterns
Leave it as is and document the lack of support for port matching in ServeMux

Not sure what the best resolution for this would be (code + doc or just doc), but any change would probably be best in 1.11 to give time for testing.

[kellegous]

Thanks for the response @kennygrant

I did read the background on the original issue. It's not clear to me yet what solution would be best for Go Authors. Like I said, since this shipped in 1.9, authors will always have to be aware of this issue. Even if 1.10 introduces a change to mitigate the impact, authors still have to consider 1.9 for quite a while.

To ignore that for a moment though. It seems like the original fix could have done a 3 step lookup for the handler, instead of simply stripping the port.

1. try to find a handler than matches Host exactly (w/ the port).
2. try to find a handler that matches Host with the port stripped (if it differs from 1., of course)
3. try to find a handler with no host specification.

Resolving from most to least specific seems in keeping with other code in ServeMux.

1. would ensure that hosts with a specific port continued to work and 2. would have fixed the issue of the busted clients that were sending :443 unnecessarily. I don't know if it's too late to rethink that change, though.

If it is too late, then it seems like mux.Handle should go ahead and strip the port since a route to localhost:8080/ is an unreachable handler in the code right now and is very likely a programmer mistake. It also seems like this deserves a bit more text in the godoc as I think most authors would expect a host route to be compatible with the HTTP Host header, right?

(I guess one thing to consider if Handle strips the port is that the workaround that I included earlier would actually cause a collision on localhost/)

The reason I ran into this is that I have a project that does depend on host routing and since the user configures those hosts, they may include ports. I plan to stop using the ServeMux in the standard lib to work around this issue.

[kennygrant]

I agree ports should be mentioned in the main servemux doc and yes if it accepts host it might seem reasonable to allow ports so it should be documented at least.

stripHostPort could maybe be moved to within mux.handler I think to do what you suggest, to have something like:

  	if mux.hosts {
		h, pattern = mux.match(host + path)
		// if no match, strip port and match again
		if h == nil {
			h, pattern = mux.match(stripHostPort(host) + path)
		}
	}

From a quick glance I think that could work - but we'd have to add some tests for hosts with port in the patterns and let others test it in the wild. That would change behaviour on connect requests though.

I'll leave it to others who know more to weigh in on their preferred solution, there is no rush if this is for 1.11.

[rsc]

At this point we shouldn't change anything, but we should document the current Go 1.9+ behavior and add tests.