request body close

2,699 views
Skip to first unread message

roger peppe

unread,
Jul 15, 2013, 5:30:18 AM7/15/13
to golang-dev
http.NewRequest coerces its body Reader to
a ReadCloser if it can, and Request.Body
is (sometimes?) closed after a request has
been made. This is undocumented
(we just had a bug because of it).

I realise we can't change the behaviour
now (though for the record I think it was probably
not a great idea), but I'd like to document the behaviour better,
and I'm not sure I understand it.

In particular I am confused by these lines,
from response.finishRequest:

// Close the body, unless we're about to close the whole TCP connection
// anyway.
if !w.closeAfterReply {
w.req.Body.Close()
}

In most cases, closeAfterReply seems to about closing the
underlying connection; what's the reasoning behind the
logic here?

I'd like to be able to write something like:

// NewRequest returns a new Request given a method, URL, and optional body.
// If body implements io.Closer, it will be closed when the request
// has been made.
func NewRequest(method, urlStr string, body io.Reader) (*Request, error)

// Post issues a POST to the specified URL.
//
// Caller should close resp.Body when done reading from it.
//
// If body implements io.Closer, it will be closed.
func (c *Client) Post(url string, bodyType string, body io.Reader)
(resp *Response, err error)

Would that documentation be correct?

Russ Cox

unread,
Jul 15, 2013, 4:53:23 PM7/15/13
to roger peppe, golang-dev
Yuck.

roger peppe

unread,
Jul 16, 2013, 4:21:32 AM7/16/13
to Russ Cox, golang-dev, Brad Fitzpatrick
any suggestions, Brad?


On 15 July 2013 21:53, Russ Cox <r...@golang.org> wrote:
> Yuck.
>

Russ Cox

unread,
Jul 16, 2013, 9:39:45 AM7/16/13
to roger peppe, golang-dev, Brad Fitzpatrick
I'm sad about the semantics but they've been there far too long to change. I think we have to document the behavior and leave it at that.

Russ

roger peppe

unread,
Jul 16, 2013, 10:21:44 AM7/16/13
to Russ Cox, golang-dev, Brad Fitzpatrick
On 16 July 2013 14:39, Russ Cox <r...@golang.org> wrote:
> I'm sad about the semantics but they've been there far too long to change. I
> think we have to document the behavior and leave it at that.

Agreed. I'm just not sure what the semantics *are*.

Brad Fitzpatrick

unread,
Jul 16, 2013, 9:35:31 PM7/16/13
to roger peppe, golang-dev
Please describe your actual bug.

I'm trying to understand how you actually hit a problem here, since you're talking about two unrelated things:  the NewRequest function for making client requests, and the internal finishRequest method on the internal response server type.  I don't believe it's possible to get a Request.Body as returned by NewRequest down into the server type, since the *Request there will always be from ReadRequest and have an internal Request.Body type (*body, etc, from transfer.go).

The reason for that "if !w.closeAfterReply" bit is to avoid reading to the end of a huge request to get to the next request in a keep-alive series of requests if we're going to hang up on the user anyway.  But that's not _your_ *Request.  It's just the same type, used in different ways.

But if Russ thinks he sees why this is "Yuck", I'm all ears.

To me, the Yuck is that we re-use the *Request type for the client and server with similar-but-different uses of 80% of the (ton of) Request fields, but that predates me.




--

---
You received this message because you are subscribed to the Google Groups "golang-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+...@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.



Russ Cox

unread,
Jul 16, 2013, 10:12:53 PM7/16/13
to Brad Fitzpatrick, roger peppe, golang-dev
The "Yuck" was in response to the fact that NewReader takes an io.Reader and, without any mention in the documentation, turns it into an io.ReadCloser. That implies to me that when you pass a reader to NewReader, eventually something calls Close on it (surprise!).

The fact of the eventual call to Close is not clear from the types (the argument is Reader, not ReadCloser) and not clear from the docs. We cannot change the behavior or the types, but we should change the docs to explain exactly when that happens.

Russ

Brad Fitzpatrick

unread,
Jul 17, 2013, 3:58:49 AM7/17/13
to Russ Cox, roger peppe, golang-dev
Sure.  Sent out https://codereview.appspot.com/11432044 to document that.

But that doesn't explain Roger's problem, as far as I can understand.

roger peppe

unread,
Jul 17, 2013, 10:17:14 AM7/17/13
to Brad Fitzpatrick, Russ Cox, golang-dev
On 17 July 2013 08:58, Brad Fitzpatrick <brad...@golang.org> wrote:
> Sure. Sent out https://codereview.appspot.com/11432044 to document that.
>
> But that doesn't explain Roger's problem, as far as I can understand.

It does explain our problem precisely. We were calling http.Put with an existing
reader (an *os.File) , and if the request failed, we were seeking to
the start of the
file and trying again - which failed.

Unfortunately the retry logic was only triggered on transient errors which were
out of the scope of our test suite, so it took a while to recognise the issue.

roger peppe

unread,
Jul 17, 2013, 10:36:48 AM7/17/13
to Brad Fitzpatrick, golang-dev
> To me, the Yuck is that we re-use the *Request type for the client and server
> with similar-but-different uses of 80% of the (ton of) Request fields, but that predates me.

Ah yes, I hadn't fully appreciated the dual-use aspect.
Reply all
Reply to author
Forward
0 new messages