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

proposal: net/http: automatically set Content-Length from Len method on Body #41182

Closed
jingyugao opened this issue Sep 2, 2020 · 7 comments
Closed
Labels
Projects
Milestone

Comments

@jingyugao
Copy link

@jingyugao jingyugao commented Sep 2, 2020

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

$ go version
go version go1.15 darwin/amd64

Does this issue reproduce with the latest release?

yes

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

go env Output
$ go env
GO111MODULE="on"
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/gaojingyu/Library/Caches/go-build"
GOENV="/Users/gaojingyu/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GONOPROXY="*.byted.org"
GONOSUMDB="*.byted.org"
GOOS="darwin"
GOPATH="/Users/gaojingyu/go"
GOPRIVATE="*.byted.org"
GOPROXY="https://go-mod-proxy.byted.org"
GOROOT="/usr/local/Cellar/go/1.13.6/libexec"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/Cellar/go/1.13.6/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/Users/gaojingyu/Tmp/go.mod"
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/r1/fgd1nc4s32zcy8myvl17np8r0000gp/T/go-build991311603=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

Now in http.NewRequestWithContext
it check wether body type is a *bytes.Buffer,*bytes.Reader or *strings.Reader, then call Len() to get content-length.

What did you expect to see?

Check wether the body type implement interfae{Len()int}

@gopherbot gopherbot added this to the Proposal milestone Sep 2, 2020
@gopherbot gopherbot added the Proposal label Sep 2, 2020
@rsc
Copy link
Contributor

@rsc rsc commented Sep 2, 2020

We know that Len is the right choice for those three types.
There's no widespread "Len-er" interface in Go, though, so I am skeptical of calling it always.
Maybe on some types Len might be a number of records?

@jingyugao
Copy link
Author

@jingyugao jingyugao commented Sep 3, 2020

We know that Len is the right choice for those three types.
There's no widespread "Len-er" interface in Go, though, so I am skeptical of calling it always.
Maybe on some types Len might be a number of records?

How about check ContentLength is zero?
If ContentLength is zero and body implement interfae{Len()int},then assign Len() to ContentLength.

@rsc rsc changed the title proposal:net/http:support get content-length from interface{Len() int} proposal: net/http: automatically set Content-Length from Len method on Body Sep 3, 2020
@rsc
Copy link
Contributor

@rsc rsc commented Sep 3, 2020

@jingyugao I certainly agree that if the caller has explicitly set ContentLength then it should not be replaced.

But that doesn't really address my objection. Content-Length is not a hint. It is a promise, and the code should not make a promise unless it is sure it can be kept. The code is sure for the three special-case types (which are very common). The code cannot be anywhere near as sure about any type that happens to have a Len() int method. Len is measured in elements, which does not always mean bytes. (For example, suppose io.MultiReader returned a concrete implementation with a Len method that returned the number of underlying readers.)

Is there some widespread body implementation you are concerned about handling?

@rsc rsc added this to Active in Proposals Sep 18, 2020
@rsc
Copy link
Contributor

@rsc rsc commented Sep 23, 2020

Adding to the proposal minutes. Len() int still seems like not enough signal for me.

@bcmills
Copy link
Member

@bcmills bcmills commented Sep 23, 2020

In #16474 (comment), @ianlancetaylor observed:

It seems that the best way to use this technique effectively is for each magic method to mean exactly one thing. … If multiple functions support the same magic method, we wind up with an N-x-N crossbar: trying to use the magic method for multiple different purposes, when we have to know which purpose is intended to know whether it makes sense.

For this case I would share Russ's concern about Len indicating the length of some container in units other than bytes — perhaps runes, number of entries, or similar.

@rsc
Copy link
Contributor

@rsc rsc commented Sep 30, 2020

Based on the discussion above, this seems like a likely decline.

@rsc rsc moved this from Active to Likely Decline in Proposals Sep 30, 2020
@rsc
Copy link
Contributor

@rsc rsc commented Oct 7, 2020

No change in consensus, so declined.

@rsc rsc closed this Oct 7, 2020
@rsc rsc moved this from Likely Decline to Declined in Proposals Oct 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Proposals
Declined
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants