Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
kellegous opened this issue Jan 5, 2018 · 13 comments
Closed

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

kellegous opened this issue Jan 5, 2018 · 13 comments
Labels
Documentation FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done. Testing An issue that has been verified to require only test changes, not just a test failure.
Milestone

Comments

@kellegous
Copy link

kellegous commented Jan 5, 2018

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
Copy link
Contributor

bradfitz commented Jan 5, 2018

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
Copy link
Author

@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
Copy link
Contributor

bradfitz commented Jan 5, 2018

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
Copy link
Contributor

bradfitz commented Jan 5, 2018

Could you report your results with Go 1.10?

@bradfitz bradfitz added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Jan 5, 2018
@kellegous
Copy link
Author

kellegous commented Jan 5, 2018

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
Copy link
Author

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))
}

@bradfitz bradfitz added NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. and removed WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. labels Jan 5, 2018
@bradfitz bradfitz added this to the Go1.11 milestone Jan 5, 2018
@kennygrant
Copy link
Contributor

kennygrant commented Jan 5, 2018

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
Copy link
Author

kellegous commented Jan 5, 2018

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
Copy link
Contributor

kennygrant commented Jan 5, 2018

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.

@odeke-em odeke-em changed the title Host routing in http.ServeMux no longer works with hosts with ports. net/http: Host routing in ServeMux no longer works on hosts with ports since Go1.9 Jan 6, 2018
@rsc
Copy link
Contributor

rsc commented Jan 29, 2018

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

@rsc rsc changed the title net/http: Host routing in ServeMux no longer works on hosts with ports since Go1.9 net/http: document and test behavior of ServeMux with ports Jan 29, 2018
@rsc rsc added the NeedsFix The path to resolution is known, but the work has not been done. label Jan 29, 2018
@gopherbot gopherbot removed the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Jan 29, 2018
@bradfitz bradfitz added Testing An issue that has been verified to require only test changes, not just a test failure. Documentation help wanted labels Jan 29, 2018
@ghost
Copy link

ghost commented Jun 20, 2018

Hi folks, I'd love to give this one a shot for my first contribution. Unless anybody here would rather do it themselves, I should have a CL coming up soon. Thanks!

@agnivade
Copy link
Contributor

Please feel free to send the CL. Thanks for your time.

@gopherbot
Copy link

Change https://golang.org/cl/120557 mentions this issue: net/http: document and test behavior of ServeMux with ports

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Documentation FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done. Testing An issue that has been verified to require only test changes, not just a test failure.
Projects
None yet
Development

No branches or pull requests

6 participants