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

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

Comments

@wujieliulan
Copy link

@wujieliulan wujieliulan commented Dec 9, 2016

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

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Dec 9, 2016

Fixed previously by golang/net@3bafa33

@bradfitz bradfitz closed this Dec 9, 2016
@wujieliulan

This comment has been minimized.

Copy link
Author

@wujieliulan wujieliulan commented Dec 9, 2016

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

This comment has been minimized.

Copy link
Contributor

@bradfitz 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.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.