-
Notifications
You must be signed in to change notification settings - Fork 18k
net/http: add MaxBytesHandler #48104
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
Conversation
This PR (HEAD: 6435fd5) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/c/go/+/346569 to see it. Tip: You can toggle comments from me using the |
Message from Damien Neil: Patch Set 1: Run-TryBot+1 Trust+1 Please don’t reply on this GitHub thread. Visit golang.org/cl/346569. |
Message from Damien Neil: Patch Set 2: Patch Set 1 was rebased Please don’t reply on this GitHub thread. Visit golang.org/cl/346569. |
Message from Damien Neil: Patch Set 2: Run-TryBot+1 Trust+1 Please don’t reply on this GitHub thread. Visit golang.org/cl/346569. |
Message from Go Bot: Patch Set 2: (1 comment) Please don’t reply on this GitHub thread. Visit golang.org/cl/346569. |
Message from Go Bot: Patch Set 2: TryBot-Result+1 (1 comment) Please don’t reply on this GitHub thread. Visit golang.org/cl/346569. |
Message from Damien Neil: Patch Set 2: Code-Review+2 Please don’t reply on this GitHub thread. Visit golang.org/cl/346569. |
Message from Ian Lance Taylor: Patch Set 2: Trust+1 Please don’t reply on this GitHub thread. Visit golang.org/cl/346569. |
Message from Ian Lance Taylor: Patch Set 2: (1 comment) Please don’t reply on this GitHub thread. Visit golang.org/cl/346569. |
Fixes #39567 Change-Id: I226089b678a6a13d7ce69f360a23fc5bd297d550 GitHub-Last-Rev: 6435fd5 GitHub-Pull-Request: #48104 Reviewed-on: https://go-review.googlesource.com/c/go/+/346569 Trust: Damien Neil <[email protected]> Trust: Ian Lance Taylor <[email protected]> Run-TryBot: Damien Neil <[email protected]> TryBot-Result: Go Bot <[email protected]> Reviewed-by: Damien Neil <[email protected]>
This PR is being closed because golang.org/cl/346569 has been merged. |
// MaxBytesHandler returns a Handler that runs h with its ResponseWriter and Request.Body wrapped by a MaxBytesReader. | ||
func MaxBytesHandler(h Handler, n int64) Handler { | ||
return HandlerFunc(func(w ResponseWriter, r *Request) { | ||
r2 := *r |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, may I ask you what's the purpose of copying request here?
I already use my own implementation in middleware like this, for a while already, and didn't get into any issues so far:
func maxBodyMiddleware(next http.Handler) http.Handler {
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
r.Body = http.MaxBytesReader(w, r.Body, int64(cfg.MaxBodySize))
next.ServeHTTP(w, r)
})
}
so would like to know if there are some edge cases which I missed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the docs for http.Handler
,
Except for reading the body, handlers should not modify the provided Request.
You will often be fine modifying a request, but you aren’t supposed to do it because it might be a race condition if some other middleware happens to read the request at the same time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, thank you!
Fixes #39567 Change-Id: I226089b678a6a13d7ce69f360a23fc5bd297d550 GitHub-Last-Rev: 6435fd5881fc70a276d04df5a60440e365924b49 GitHub-Pull-Request: golang/go#48104 Reviewed-on: https://go-review.googlesource.com/c/go/+/346569 Trust: Damien Neil <[email protected]> Trust: Ian Lance Taylor <[email protected]> Run-TryBot: Damien Neil <[email protected]> TryBot-Result: Go Bot <[email protected]> Reviewed-by: Damien Neil <[email protected]>
Fixes #39567