Go version

go version devel go1.24-b17a55d095 Tue Sep 24 23:20:50 2024 +0000 linux/amd64

Output of go env in your module/workspace:

GO111MODULE=''
GOARCH='amd64'
GOBIN=''
GOCACHE='/home/chressie/.cache/go-build'
GOENV='/home/chressie/.config/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='amd64'
GOHOSTOS='linux'
GOINSECURE=''
GOMODCACHE='/home/chressie/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='linux'
GOPATH='/home/chressie/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/home/chressie/src/go'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/home/chressie/src/go/pkg/tool/linux_amd64'
GOVCS=''
GOVERSION='devel go1.24-b17a55d095 Tue Sep 24 23:20:50 2024 +0000'
GODEBUG=''
GOTELEMETRY='on'
GOTELEMETRYDIR='/home/chressie/.config/go/telemetry'
GCCGO='/usr/bin/gccgo'
GOAMD64='v1'
AR='ar'
CC='gcc'
CXX='g++'
CGO_ENABLED='1'
GOMOD='/home/chressie/src/go/src/go.mod'
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=/tmp/go-build549655017=/tmp/go-build -gno-record-gcc-switches'

What did you do?

The http.ServeMux.Handler method currently discards patterns and matches that findHandler returns.

This behavior makes it impossible to use named path wildcards in http.Handler implementations that want to use an http.ServeMux as the underlying request multiplexer.

The following program shows a common pattern that demonstrates this:

package main

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

func main() {
    mux := http.NewServeMux()
    mux.HandleFunc("/{foo}", func(w http.ResponseWriter, r *http.Request) {
        fmt.Printf("handler1: {foo}==%q\n", r.PathValue("foo"))
    })
    mux.HandleFunc("/x/{bar}", func(w http.ResponseWriter, r *http.Request) {
        fmt.Printf("handler2: {bar}==%q\n", r.PathValue("bar"))
    })

    // Typical middleware that routes all requests to mux.
    http.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) {
        h, _ := mux.Handler(r)
        h.ServeHTTP(w, r)
    })

    ln, _ := net.Listen("tcp", ":0")
    go (&http.Server{Handler: http.DefaultServeMux, Addr: ln.Addr().String()}).Serve(ln)
    http.Get("http://" + ln.Addr().String() + "/wildcard-foo")
    http.Get("http://" + ln.Addr().String() + "/x/wildcard-bar")
}

What did you see happen?

Executing above program yields

% go run t.go
handler1: {foo}==""
handler2: {bar}==""

The wildcards were not populated.

What did you expect to see?

I'd expect to see the wildcards populated.

% go run t.go
handler1: {foo}=="wildcard-foo"
handler2: {bar}=="wildcard-bar"

Comment From: gabyhelp

Related Issues and Documentation

(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)

Comment From: chressie

I drafted a possible fix in https://go.dev/cl/615735.

Comment From: gopherbot

Change https://go.dev/cl/615735 mentions this issue: net/http: populate named path wildcards into Request in ServeMux.Handler

Comment From: seankhliao

cc @jba @neild

imo this is working as intended and finding a handler shouldn't modify the request.

Comment From: neild

It doesn't seem unreasonable for ServeMux.Handler to populate the request path. But on the other hand, it doesn't seem unreasonable to say that it's surprising for a lookup operation to modify the lookup key. So I don't know.

Unless I'm missing something, the example middleware can be written more simply using ServeMux.ServeHTTP, which will populate the path as desired:

http.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) {
    mux.ServeHTTP(w, r)
})

Comment From: chressie

Thanks for your thoughts. If one thinks of ServeMux.Handler as a pure lookup function, then i agree that populating the request is surprising.

In my mind the bug is that the returned handler can no longer be used as a regular HTTP handler. That means it's no longer safe to call ServeHTTP on it, because the request doesn't carry the information it needs (at least when using patterns with wildcards).

The example middleware in the initial description was a bit too short to demonstrate this, so here's a slightly extended version that shows how to subtly hold it wrong:

http.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) {
    h, _ := mux.Handler(r)
    if _, ok := h.(*internalAccessHandler); ok {
        h = ensureAuthenticationHandler{h}
    }
    h.ServeHTTP(w, r)
})

At a first glance the code looks fine, but assuming ensureAuthenticationHandler.ServeHTTP eventually calls h.ServeHTTP it would fail on patterns with wildcards. The fix is, as you suggest, to re-use the mux (e.g. ensureAuthenticationHandler{mux}), but that's very subtle.

I think it would be better to populate the request and avoid subtle bugs like the above.

Comment From: seankhliao

i still think Handler should be a pure lookup function, and it's up to the middleware to use the second return parameter to fill in the request, as is expected of any other third party router.

Comment From: dr2chase

unclear if this "needs investigation" or WAI.

Comment From: jba

Definitely "needs investigation." It's not clear to me what the implications are of populating the request in the presence of further mux nesting and http.StripPrefix. Also, to echo what @seankhliao said, there may be other uses of Handler that are just pure lookup, perhaps with variations on the request. The fundamental problem is that you're modifying a shared resource. Furthermore, so far in all examples, there is a workaround that is not too far from the problematic code (though perhaps a bit subtle).

Comment From: neild

Maybe the handler returned by mux.Handler should modify the request passed to it?

http.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) {
    // mux.Handler does not modify r.
    h, _ := mux.Handler(r)

    // But h.ServeHTTP does.
    h.ServeHTTP(w, r)
})

Comment From: chressie

Wrapping the registered handler into another handler (which then modifies the request to populate the fields on demand) would break code that expects the returned handler to match the registered one (e.g. uses type assertion).

The options that i'm seeing right now:

  • Accept the current state and document that calling ServeHTTP doesn't work with patterns with wildcards and that the original mux should be used.
  • Populate the request.
  • A variant could be to just modify the request when there actually was a wildcard in the pattern and otherwise leave it as is.
  • Have an extra API that allows the request to be populated.

I don't find any of these particularly appealing. Maybe there are more options that i'm not seeing though.

Comment From: gopherbot

Change https://go.dev/cl/618096 mentions this issue: net/http: document ServeMux.Handler as a pure lookup function

Comment From: siggimoo

The lack of this capability is the only reason I'm using Gorilla Mux for my current project. It provides path values both in the matching result and from a separate function. I don't care which approach the standard library adopts, but it should have one of them.

Comment From: jba

I see why this feature is useful for some programs.

It's not acceptable for Handler to populate the request. It violates the implicit contract that Handler is read-only, and is not backwards-compatible.

That leaves the idea of adding another method, like Handler but with the additional behavior of populating the request.

The only hard part is the name. Some thoughts: - HandlerSetRequest - HandlerSetMatches - HandlerWithMatches - Handler{Set,With}Wildcards

I don't love any of these.

Comment From: FiloSottile

Since the missing bit of information is retrieved with Request.PathValue and set with Request.SetPathValue... HandlerWithPathValues?

Comment From: jba

I think @FiloSottile's name is fine. We should pick a name and get this accepted if it's what people want. I think it's still an open question whether this will help buggy programs like https://github.com/golang/go/issues/69623#issuecomment-2377062875. @chressie, do you think having a different method that will show up next to ServeMux.Handler in the docs, plus a comment on Handler that it doesn't populate the request, is better than the status quo?

Comment From: chressie

Since i opened this issue i haven't heard from anybody else that the read-only behavior is problematic. I think that's enough evidence at this point that an additional method is actually not justified. Just updating the comment Handler method's comment to say that wildcards are not populated is probably sufficient.

Comment From: gopherbot

Change https://go.dev/cl/674095 mentions this issue: src/net/http: clarify ServeMux.Handler behavior

Comment From: SampsonCrowley

i still think Handler should be a pure lookup function, and it's up to the middleware to use the second return parameter to fill in the request, as is expected of any other third party router.

@seankhliao except that means that the work that was already done by the ServeMux findHandler function is wasted. I think it's much more surprising that finding the handler then is not usable with the request

this requires devs to duplicate work and logic that is already written by the upstream because it's also impossible to not only get the data without reparsing yourself, but it's impossible to assign the matches and actual pattern (not just the pattern string but the object that already contains the matched segments) because Request.pat and Request.matches are both private properties.

the only option is to reparse and then set the same properties into the "other" matches one-by-one via SetPathValu

we can't both keep the required pieces from being assignable and not fill them out...

Comment From: SampsonCrowley

I can't think of any good reason why we can't have a second function that does modify the request so that we're not changing current behavior of Handler, but we at least give end-users a way to extend the built-in mux without duplicating logic that is currently forced to be inaccessible by making it private

it could be the exact same logic as what's already used at the end of ServeHTTP, except moved into a separate function. then ServeHTTP can call that function in it's processing

doesn't break anything, solves the use case for everyone who wants it, nobody has to use it