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

Jump to bottom

x/net/http2: #18258

Closed
wujieliulan opened this issue Dec 9, 2016 · 3 comments
Closed

x/net/http2: #18258

wujieliulan opened this issue Dec 9, 2016 · 3 comments
Labels
FrozenDueToAge

Comments

@wujieliulan
Copy link

wujieliulan commented Dec 9, 2016 •
edited

Please answer these questions before submitting your issue. Thanks!

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

go1.7.3 linux/amd64

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

GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/root/go"
GORACE=""
GOROOT="/usr/local/go"
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
CC="gcc"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build560134607=/tmp/go-build"
CXX="g++"
CGO_ENABLED="1"

What did you do?

If possible, provide a recipe for reproducing the error.
A complete runnable program is good.
A link on play.golang.org is best.
http2 server getRequestBodyBuf() creates lots garbage and memory usage is high

What did you expect to see?

What did you see instead?

Most requests don't have body, but all allocates a 64K buffer, no request body read is called , therefore, putRequestBodyBuf() won't be called to recycle the buffer.

Is it possible to allocate 64k buffer only if there is a request body?
@bradfitz
Copy link
Contributor

bradfitz commented Dec 9, 2016 •
edited

Fixed previously by golang/net@3bafa33

@bradfitz bradfitz closed this as completed Dec 9, 2016
@wujieliulan
Copy link
Author

wujieliulan commented Dec 9, 2016 •
edited

with golang/net@3bafa33 it still creates lots of garbage, the reason is for requests with no body, putRequestBodyBuf() is never called. Calling it in handlerDone() helps, but it still wastes lots of 64k buffer for requests without body (which is majority). I use this workaround, it saves lots of memory with busy servers:

--- a/http2/server.go
+++ b/http2/server.go
@@ -1418,13 +1418,12 @@ func (sc *serverConn) processData(f *DataFrame) error {
 
                return streamError(id, ErrCodeStreamClosed)
        }
-       if st.body == nil {
-               panic("internal error: should have a body in this state")
-       }
 
        // Sender sending more than they'd declared?
        if st.declBodyBytes != -1 && st.bodyBytes+int64(len(data)) > st.declBodyBytes {
-               st.body.CloseWithError(fmt.Errorf("sender tried to send more than declared Content-Length of %d bytes", st.declBody
+               if st.body != nil {
+                       st.body.CloseWithError(fmt.Errorf("sender tried to send more than declared Content-Length of %d bytes", st.
+               }
                return streamError(id, ErrCodeStreamClosed)
        }
        if f.Length > 0 {
@@ -1435,6 +1434,16 @@ func (sc *serverConn) processData(f *DataFrame) error {
                st.inflow.take(int32(f.Length))
 
                if len(data) > 0 {
+                       reqbodyMu.Lock()
+                       if st.body == nil {
+                               st.reqBuf = getRequestBodyBuf()
+                               st.body = &pipe{
+                                       b: &fixedBuffer{buf: st.reqBuf},
+                               }
+                               //panic("internal error: should have a body in this state")
+                       }
+                       reqbodyMu.Unlock()
                        wrote, err := st.body.Write(data)
                        if err != nil {
                                return streamError(id, ErrCodeStreamClosed)
@@ -1483,12 +1492,14 @@ func (st *stream) endStream() {
        sc := st.sc
        sc.serveG.check()
 
-       if st.declBodyBytes != -1 && st.declBodyBytes != st.bodyBytes {
-               st.body.CloseWithError(fmt.Errorf("request declared a Content-Length of %d but only wrote %d bytes",
-                       st.declBodyBytes, st.bodyBytes))
-       } else {
-               st.body.closeWithErrorAndCode(io.EOF, st.copyTrailersToHandlerRequest)
-               st.body.CloseWithError(io.EOF)
+       if st.body != nil {
+               if st.declBodyBytes != -1 && st.declBodyBytes != st.bodyBytes {
+                       st.body.CloseWithError(fmt.Errorf("request declared a Content-Length of %d but only wrote %d bytes",
+                               st.declBodyBytes, st.bodyBytes))
+               } else {
+                       st.body.closeWithErrorAndCode(io.EOF, st.copyTrailersToHandlerRequest)
+                       st.body.CloseWithError(io.EOF)
+               }
        }
        st.state = stateHalfClosedRemote
 }
@@ -1741,11 +1752,6 @@ func (sc *serverConn) newWriterAndRequest(st *stream, f *MetaHeadersFrame) (*res
                return nil, nil, err
        }
        if bodyOpen {
-               st.reqBuf = getRequestBodyBuf()
-               req.Body.(*requestBody).pipe = &pipe{
-                       b: &fixedBuffer{buf: st.reqBuf},
-               }
-
                if vv, ok := rp.header["Content-Length"]; ok {

@bradfitz
Copy link
Contributor

bradfitz commented Dec 9, 2016

To send changes, see https://golang.org/doc/contribute.html

We can't accept diffs over email or github.

@golang golang locked and limited conversation to collaborators Dec 9, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@bradfitz @wujieliulan @gopherbot