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: panic: runtime error: comparing uncomparable type when the underlying type has a map #29768

Closed
vadmeste opened this issue Jan 16, 2019 · 4 comments

Comments

Projects
None yet
3 participants
@vadmeste
Copy link

commented Jan 16, 2019

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

go version go1.10.4 linux/amd64

Does this issue reproduce with the latest release?

Not sure, the error is too rare for me but it seems the reason of the error is easy to discern.

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

GOARCH="amd64"
GOBIN=""
GOCACHE="/home/vadmeste/.cache/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/vadmeste/work/gospace"
GOPROXY=""
GORACE=""
GOROOT="/home/vadmeste/work/go"
GOTMPDIR=""
GOTOOLDIR="/home/vadmeste/work/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD=""
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 -fmessage-length=0 -fdebug-prefix-map=/mnt/p4/vadmeste/tmp/go-build653380043=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Still, the error is so rare to be reproduced but it seems the reason of the error is easy to discern.

What did you expect to see?

No panic

What did you see instead?

panic: runtime error: comparing uncomparable type minio.ErrorResponse

goroutine 352943 [running]:
net/http.(*Request).write(0xc4229baf00, 0xcadce0, 0xc42179c840, 0x0, 0x0, 0x0, 0xcaf100, 0xc4208d4280)
    /opt/go/src/net/http/request.go:624 +0x71b
net/http.(*persistConn).writeLoop(0xc421cf3200)
    /opt/go/src/net/http/transport.go:1825 +0x1ea
created by net/http.(*Transport).dialConn
    /opt/go/src/net/http/transport.go:1238 +0x97f

More information

The following is the definition of minio.ErrorResponse (https://github.com/minio/minio-go/blob/master/api-error-response.go#L38)

// ErrorResponse - Is the typed error returned by all API operations.
type ErrorResponse struct {
	XMLName    xml.Name `xml:"Error" json:"-"`
	Code       string
	Message    string
	BucketName string
	Key        string
	RequestID  string `xml:"RequestId"`
	HostID     string `xml:"HostId"`

	// Region where the bucket is located. This header is returned
	// only in HEAD bucket and ListObjects response.
	Region string

	// Underlying HTTP status code for the returned error
	StatusCode int `xml:"-" json:"-"`

	// Headers of the returned S3 XML error
	Headers http.Header `xml:"-" json:"-"`
}
@agnivade

This comment has been minimized.

Copy link
Member

commented Jan 18, 2019

Yes, this is because ErrorResponse has Headers which is actually a map. And maps are not comparable.

So, when both bodyReadError and err are internally of type ErrorResponse, the comparison crashes.

if tw.bodyReadError == err {
	err = requestBodyReadError{err}
}

https://github.com/golang/go/blob/master/src/net/http/request.go#L652

A small reproducer -

package main

import (
	"fmt"
	"net/http"
)

type ErrorResponse struct {
	Message string
	Headers http.Header `xml:"-" json:"-"`
}

// Error - Returns S3 error string.
func (e ErrorResponse) Error() string {
	return e.Message
}

func main() {
	hh := make(http.Header)
	hh.Add("hi", "kk")
	var e1 error = ErrorResponse{
		Headers: hh,
	}
	fmt.Println(e1 == e1)
}

@bradfitz - thoughts ? Since we are not in control of the underlying type, things like these can happen.
reflect.DeepEqual does bypass the panic, but do we want something like that inside net/http ?

@agnivade agnivade changed the title Comparing uncomparable type panic inside golang http net/http: panic: runtime error: comparing uncomparable type when the underlying type has a map Jan 18, 2019

@agnivade agnivade added this to the Go1.13 milestone Jan 18, 2019

@bradfitz

This comment has been minimized.

Copy link
Member

commented Jan 18, 2019

Yeah, sorry, maps aren't comparable. This isn't an HTTP issue, really.

@bradfitz bradfitz closed this Jan 18, 2019

@vadmeste

This comment has been minimized.

Copy link
Author

commented Jan 18, 2019

Someone would easily define an error struct which has a map or a slice inside it to facilitate the construction of the error message.

Since we cannot write a custom comparator in Golang for my custom error struct (ErrorResponse), it doesn't seem there is a way to avoid the panic without restructuring my code.

With this additional context description, @bradfitz do you still feel this is not a http pkg issue ?

@bradfitz

This comment has been minimized.

Copy link
Member

commented Jan 18, 2019

My opinion is unchanged. You'll have to restructure your code so you don't do things not allowed by the language.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.