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.

Sign up for GitHub

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

Jump to bottom

expvar: creates conflict with "GET /" route #65723

Closed
xpmatteo opened this issue Feb 15, 2024 · 12 comments
Closed

expvar: creates conflict with "GET /" route #65723

xpmatteo opened this issue Feb 15, 2024 · 12 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.

Comments

@xpmatteo
Copy link

Go version

1.22.0

Output of go env in your module/workspace:

GO111MODULE=''
GOARCH='amd64'
GOBIN=''
GOCACHE='/Users/matteo/Library/Caches/go-build'
GOENV='/Users/matteo/Library/Application Support/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='amd64'
GOHOSTOS='darwin'
GOINSECURE=''
GOMODCACHE='/Users/matteo/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='darwin'
GOPATH='/Users/matteo/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/usr/local/Cellar/go/1.22.0/libexec'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/usr/local/Cellar/go/1.22.0/libexec/pkg/tool/darwin_amd64'
GOVCS=''
GOVERSION='go1.22.0'
GCCGO='gccgo'
GOAMD64='v1'
AR='ar'
CC='cc'
CXX='c++'
CGO_ENABLED='1'
GOMOD='/dev/null'
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 -arch x86_64 -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -ffile-prefix-map=/var/folders/tb/wk8169j96gbcyz_jrppmchvm0000gn/T/go-build2446679495=/tmp/go-build -gno-record-gcc-switches -fno-common'

What did you do?

With the new routing style in go 1.22, declaring

    http.Handle("GET /", h)

generates a conflict with route "/debug/vars" declared in the expvar package.

What did you see happen?

panic: pattern "GET /" (registered at ...) conflicts with pattern "/debug/vars" (registered at ...expvar.go:384):
GET / matches fewer methods than /debug/vars, but has a more general path pattern

What did you expect to see?

No error.

It can be fixed with this patch

--- a/src/expvar/expvar.go
+++ b/src/expvar/expvar.go
@@ -381,7 +381,7 @@ func memstats() any {
 }

 func init() {
-       http.HandleFunc("/debug/vars", expvarHandler)
+       http.HandleFunc("GET /debug/vars", expvarHandler)
        Publish("cmdline", Func(cmdline))
        Publish("memstats", Func(memstats))
 }
@xpmatteo
Copy link
Author

I expect that similar problems will be caused by the many other places in the std lib where http.HandleFunc is invoked with the old syntax

@gopherbot
Copy link

Change https://go.dev/cl/564435 mentions this issue: expvar: avoid conflict with a user-defined "GET /" route. Fixes #65723

@mateusz834
Copy link
Member

I guess it needs also to check whether the GODEBUG httpmuxgo121 == 1, so that it still works for code that runs with httpmuxgo121=0.

xpmatteo pushed a commit to xpmatteo/go that referenced this issue Feb 16, 2024
@xpmatteo
expvar: avoid conflict with "GET /" route; fixes golang#65723
de3dbf1 
With the new routing style in go 1.22, declaring

    http.Handle("GET /", h)

generates a conflict with route "/debug/vars" declared in the expvar package.
You get an error such as:

panic: pattern "GET /" (registered at ...) conflicts with pattern "/debug/vars" (registered at ...expvar.go:384):
GET / matches fewer methods than /debug/vars, but has a more general path pattern

This patch prevents that error.  Adding GET is correct because no other method makes
sense with /debug/vars.

We preserve the traditional behaviour when GODEBUG=httpmuxgo121=1 is specified.
xpmatteo pushed a commit to xpmatteo/go that referenced this issue Feb 16, 2024
@xpmatteo
expvar: avoid conflict with "GET /" route. Fixes golang#65723
ff3d530 
With the new routing style in go 1.22, declaring

    http.Handle("GET /", h)

generates a conflict with route "/debug/vars" declared in the expvar package.
You get an error such as:

panic: pattern "GET /" (registered at ...) conflicts with pattern "/debug/vars" (registered at ...expvar.go:384):
GET / matches fewer methods than /debug/vars, but has a more general path pattern

This patch prevents that error.  Adding GET is correct because no other method makes
sense with /debug/vars.

We preserve the traditional behaviour when GODEBUG=httpmuxgo121=1 is specified.
@xpmatteo
Copy link
Author

I guess it needs also to check whether the GODEBUG httpmuxgo121 == 1, so that it still works for code that runs with httpmuxgo121=0.

Indeed, this is needed, otherwise specifying GODEBUG httpmuxgo121=1 would make /debug/vars no longer available. I updated the patch accordingly.

(I closed the previous PR by mistake, so I had to open a new one. Newbie mistake!)

@gopherbot
Copy link

Change https://go.dev/cl/564735 mentions this issue: expvar: avoid conflict with user-defined "GET /" route. Fixes #65723

@thanm thanm added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Feb 16, 2024
@thanm
Copy link
Contributor

thanm commented Feb 16, 2024

@bradfitz per owners

@AlexanderYastrebov
Copy link
Contributor

AlexanderYastrebov commented Feb 16, 2024 •
edited

@jba FYI

@seankhliao
Copy link
Member

net/http/pprof will also have a similar issue.

@jba
Copy link
Contributor

jba commented Feb 16, 2024

@xpmatteo, would you like to handle net/http/pprof while you're at it?

@xpmatteo
Copy link
Author

@xpmatteo, would you like to handle net/http/pprof while you're at it?

I will do it; should I open a separate PR?

@jba
Copy link
Contributor

jba commented Feb 16, 2024

Yes please.

xpmatteo pushed a commit to xpmatteo/go that referenced this issue Feb 16, 2024
@xpmatteo
expvar: avoid conflict with user-defined "GET /" route.
5f21a97 
With the new routing style in go 1.22, declaring

    http.Handle("GET /", h)

generates a conflict with route "/debug/vars" declared in the expvar package.
You get an error such as:

panic: pattern "GET /" (registered at ...) conflicts with pattern "/debug/vars" (registered at ...expvar.go:384):
GET / matches fewer methods than /debug/vars, but has a more general path pattern

This patch prevents that error.  Adding GET is correct because no other method makes
sense with /debug/vars. However, a tool using any other methods will break.

We preserve the traditional behaviour when GODEBUG=httpmuxgo121=1 is specified.

Fixes golang#65723
xpmatteo added a commit to xpmatteo/go that referenced this issue Feb 19, 2024
@xpmatteo
net/http/pprof: avoid panic with user-defined "GET /" route
48ff27b 
With the new routing style in go 1.22, declaring

    http.Handle("GET /", h)

generates a conflict with route "/debug/pprof/" and the others
declared in the net/http/pprof package.
You get an error such as:

panic: pattern "GET /" (registered at .../pprof.go:94):
GET / matches fewer methods than /debug/pprof/, but has a more general path
pattern [recovered]
  panic: pattern "GET /" (registered at ...:17) conflicts with pattern
  "/debug/pprof/" (registered at .../pprof.go:94):

This patch prevents that error.  Adding GET is correct because no other
method makes sense with the /debug/pprof routes. However, a tool using
any method other than GET will break.

We preserve the traditional behaviour when GODEBUG=httpmuxgo121=1 is
specified.

This is related to issue golang#65723
xpmatteo pushed a commit to xpmatteo/go that referenced this issue Feb 19, 2024
@xpmatteo
net/http/pprof: avoid panic with user-defined "GET /" route
afc49af 
With the new routing style in go 1.22, declaring

    http.Handle("GET /", h)

generates a conflict with route "/debug/pprof/" and the others
declared in the net/http/pprof package.
You get an error such as:

panic: pattern "GET /" (registered at .../pprof.go:94):
GET / matches fewer methods than /debug/pprof/, but has a more general path
pattern [recovered]
  panic: pattern "GET /" (registered at ...:17) conflicts with pattern
  "/debug/pprof/" (registered at .../pprof.go:94):

This patch prevents that error.  Adding GET is correct because no other
method makes sense with the /debug/pprof routes. However, a tool using
any method other than GET will break.

We preserve the traditional behaviour when GODEBUG=httpmuxgo121=1 is
specified.

This is related to issue golang#65723
This was referenced Feb 19, 2024
Closed
Closed
@gopherbot
Copy link

Change https://go.dev/cl/565176 mentions this issue: net/http/pprof: avoid panic with user-defined "GET /" route

xpmatteo pushed a commit to xpmatteo/go that referenced this issue Feb 21, 2024
net/http/pprof: avoid panic with user-defined "GET /" route
8939266 
With the new routing style in go 1.22, declaring

    http.Handle("GET /", h)

generates a conflict with route "/debug/pprof/" and the others
declared in the net/http/pprof package.
You get an error such as:

panic: pattern "GET /" (registered at .../pprof.go:94):
GET / matches fewer methods than /debug/pprof/, but has a more general path
pattern [recovered]

This patch prevents that error.  Adding GET is correct because no other
method makes sense with the /debug/pprof routes. However, a tool using
any method other than GET will break.

We preserve the traditional behaviour when GODEBUG=httpmuxgo121=1 is
specified.

Updates golang#65723
xpmatteo pushed a commit to xpmatteo/go that referenced this issue Feb 22, 2024
net/http/pprof: avoid panic with user-defined "GET /" route
35e4012 
With the new routing style in go 1.22, declaring

    http.Handle("GET /", h)

generates a conflict with route "/debug/pprof/" and the others
declared in the net/http/pprof package.
You get an error such as:

panic: pattern "GET /" (registered at .../pprof.go:94):
GET / matches fewer methods than /debug/pprof/, but has a more general
path pattern [recovered]

This patch prevents that error.  Adding GET is correct because no other
method makes sense with the /debug/pprof routes. However, a tool using
any method other than GET will break.

We preserve the traditional behaviour when GODEBUG=httpmuxgo121=1 is
specified.

Updates golang#65723
@xpmatteo xpmatteo mentioned this issue Feb 22, 2024
Closed
xpmatteo pushed a commit to xpmatteo/go that referenced this issue Feb 22, 2024
expvar: avoid conflict with user-defined "GET /" route.
2322758 
With the new routing style in go 1.22, declaring

    http.Handle("GET /", h)

generates a conflict with route "/debug/vars" declared in the expvar package.
You get an error such as:

panic: pattern "GET /" (registered at ...) conflicts with pattern
"/debug/vars" (registered at ...expvar.go:384):
GET / matches fewer methods than /debug/vars, but has a more general path
pattern

This patch prevents that error.  Adding GET is correct because no other
method makes sense with /debug/vars. However, a tool using any other
methods will break.

We preserve the traditional behaviour when GODEBUG=httpmuxgo121=1 is
specified.

Fixes golang#65723
xpmatteo pushed a commit to xpmatteo/go that referenced this issue Feb 22, 2024
expvar: avoid conflict with user-defined "GET /" route.
9c2b9f7 
With the new routing style in go 1.22, declaring

    http.Handle("GET /", h)

generates a conflict with route "/debug/vars" declared in the expvar
package. You get an error such as:

panic: pattern "GET /" (registered at ...) conflicts with pattern
"/debug/vars" (registered at ...expvar.go:384):
GET / matches fewer methods than /debug/vars, but has a more general path
pattern

This patch prevents that error.  Adding GET is correct because no other
method makes sense with /debug/vars. However, a tool using any other
methods will break.

We preserve the traditional behaviour when GODEBUG=httpmuxgo121=1 is
specified.

Fixes golang#65723
gopherbot pushed a commit that referenced this issue Feb 27, 2024
@jba
net/http/pprof: avoid panic with user-defined "GET /" route
9fcffc5 
With the new routing style in go 1.22, declaring

    http.Handle("GET /", h)

generates a conflict with route "/debug/pprof/" and the others declared in
the net/http/pprof package. You get an error such as:

panic: pattern "GET /" (registered at .../pprof.go:94): GET / matches
fewer methods than /debug/pprof/, but has a more general path pattern

This patch prevents that error.  Adding GET is correct because no other
method makes sense with the /debug/pprof routes. However, a tool using any
method other than GET will break.

We preserve the traditional behaviour when GODEBUG=httpmuxgo121=1 is
specified.

Updates #65723

Change-Id: I49c21f5f3e802ad7538062d824354b2e4d8a800e
GitHub-Last-Rev: 35e4012
GitHub-Pull-Request: #65791
Reviewed-on: https://go-review.googlesource.com/c/go/+/565176
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Jonathan Amsterdam <jba@google.com>
Reviewed-by: Carlos Amedee <carlos@golang.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

expvar: avoid conflict with user-defined "GET /" route. xpmatteo/go
7 participants
@xpmatteo @AlexanderYastrebov @gopherbot @seankhliao @thanm @jba @mateusz834