[go] net/http: prevent server from draining request body if status 100 was not sent
Nicholas Husin (Gerrit)
New activity on the change![]( "Open in Gerrit")
Related details
- Damien Neil
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nicholas Husin (Gerrit)
Nicholas Husin uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result+1 by golang...@luci-project-accounts.iam.gserviceaccount.com
Related details
- Damien Neil
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nicholas Husin (Gerrit)
Nicholas Husin uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result+1 by golang...@luci-project-accounts.iam.gserviceaccount.com
Related details
- Damien Neil
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nicholas Husin (Gerrit)
Nicholas Husin uploaded new patchset![]( "Open in Gerrit")
- Damien Neil
- Nicholas Husin
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Damien Neil (Gerrit)
Damien Neil added 3 comments![]( "Open in Gerrit")
w.closeAfterReply = trueCan this be overwritten by chunkWriter.writeHeader? I might be following this wrong, but I think if you call WriteHeader(200) we set w.closeAfterReply here (but the header is not written, merely buffered), and then if you call Write later writeHeader can overwrite it.
If so, I think the simpler thing might be to add another bool to response indicating whether we've written a 100-Continue, and then have response.shouldReuseConnection take it into account. Might be simplest as a response.wantsContinue, set by conn.serve at the same time it initializes canWriteContinue, and unset if we ever send a 100-Continue.
b.mu.Lock()This is going to deadlock if a body read is happening in another goroutine, which I think can't happen because any body read will have gone through expectContinueReader.Read.
But in the interest of reducing the number of layers we have to think about at once (because I confess this code confuses the heck out of me already), how about we set expectContinueReader.closed instead of body.closed? disableWriteContinue is already synchronizing with it via w.writeContinueMu so we only have one set of locks to think about.
if code < 101 || code > 199 {Let's make this `code == 100 || code >= 200` while we're touching it, checkWriteHeaderCode will have rejected <100 but the more limited check seems clearer to me. And as a really minor bit of clarity, I think "200 and higher" is a clearer expression of intent than "higher than 199".
Related details
- Nicholas Husin
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nicholas Husin (Gerrit)
Nicholas Husin uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result+1 by golang...@luci-project-accounts.iam.gserviceaccount.com
Related details
- Nicholas Husin
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nicholas Husin (Gerrit)
Nicholas Husin added 3 comments![]( "Open in Gerrit")
Can this be overwritten by chunkWriter.writeHeader? I might be following this wrong, but I think if you call WriteHeader(200) we set w.closeAfterReply here (but the header is not written, merely buffered), and then if you call Write later writeHeader can overwrite it.
If so, I think the simpler thing might be to add another bool to response indicating whether we've written a 100-Continue, and then have response.shouldReuseConnection take it into account. Might be simplest as a response.wantsContinue, set by conn.serve at the same time it initializes canWriteContinue, and unset if we ever send a 100-Continue.
I think `chunkWriter.writeHeader` overwriting `w.closeAfterReply` should be impossible. Looking at symbol references, `w.closeAfterReply` can only be set to `false` when [dealing with HTTP/1.0 keep-alive connections](https://cs.opensource.google/go/go/+/master:src/net/http/server.go;l=1360-1365;drc=57f9a589b5dfd9635879e85cd1c2937441368989). `100 Continue` is only supported in HTTP/1.1, so we should be good here.
This is going to deadlock if a body read is happening in another goroutine, which I think can't happen because any body read will have gone through expectContinueReader.Read.
But in the interest of reducing the number of layers we have to think about at once (because I confess this code confuses the heck out of me already), how about we set expectContinueReader.closed instead of body.closed? disableWriteContinue is already synchronizing with it via w.writeContinueMu so we only have one set of locks to think about.
Good idea, thanks, done!
Also explicitly make this method a noop unless `w.reqBody` is an `expectContinueReader` to hopefully make things clearer.
Let's make this `code == 100 || code >= 200` while we're touching it, checkWriteHeaderCode will have rejected <100 but the more limited check seems clearer to me. And as a really minor bit of clarity, I think "200 and higher" is a clearer expression of intent than "higher than 199".
- Damien Neil
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Damien Neil (Gerrit)
Damien Neil voted Code-Review+2![]( "Open in Gerrit")
- Nicholas Husin
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |