Request Content Length zero when copy from another Request

196 views
Skip to first unread message

matteo....@gmail.com

unread,
Feb 5, 2019, 11:12:36 AM2/5/19
to golang-nuts
I've the following situation: 
I proxy a request to another server and when I made a POST and create a new request, the contentLength is zero:

        req2, _ := http.NewRequest(req.Method, newApiUrl , req.Body)
        fmt.Println("New request from body:", req2.ContentLength) // print 0

Checking in the source code of the NewRequest func Body don't respect some interface and populate the ContentLength field.

Could be a bug? Which could be a valid approach in order to create a new request from an existing one and correct set the Body length?


Thanks!

Burak Serdar

unread,
Feb 5, 2019, 11:31:42 AM2/5/19
to matteo....@gmail.com, golang-nuts
On Tue, Feb 5, 2019 at 9:12 AM <matteo....@gmail.com> wrote:
>
> I've the following situation:
> I proxy a request to another server and when I made a POST and create a new request, the contentLength is zero:
>
> req2, _ := http.NewRequest(req.Method, newApiUrl , req.Body)
> fmt.Println("New request from body:", req2.ContentLength) // print 0
>
> Checking in the source code of the NewRequest func Body don't respect some interface and populate the ContentLength field.

When the first request is created, req.Body is set to a NopCloser, and
that doesn't match any of the cases in the second NewRequest. For this
to work, I guess the best way is to get the body length and set it
explicitly after constructing the second request.

I don't know if this is intentional or not, but in general, it might
be a good idea to add a ReaderWrapper interface to the standard
library that defines a GetNestedReader() io.Reader function to access
the real reader from these utility classes.

>
> Could be a bug? Which could be a valid approach in order to create a new request from an existing one and correct set the Body length?
>
> A working example here:
>
> https://play.golang.org/p/SvCDLj0NrXb
>
> Thanks!
>
> --
> You received this message because you are subscribed to the Google Groups "golang-nuts" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to golang-nuts...@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.

Robert Engels

unread,
Feb 5, 2019, 11:41:49 AM2/5/19
to Burak Serdar, matteo....@gmail.com, golang-nuts
GetNested anything is a sign something is broken in the API. The whole point of being nested is almost always encapsulation and then you are breaking it.

Burak Serdar

unread,
Feb 5, 2019, 11:46:46 AM2/5/19
to Robert Engels, matteo....@gmail.com, golang-nuts
On Tue, Feb 5, 2019 at 9:41 AM Robert Engels <ren...@ix.netcom.com> wrote:
>
> GetNested anything is a sign something is broken in the API. The whole point of being nested is almost always encapsulation and then you are breaking it.

That's too much of a generalization in my opinion. This is more like
decoration, adding new functionality at every layer, but there is no
way to expose all the functionality of the nested thing, so you need
to expose the thing itself.

Robert Engels

unread,
Feb 5, 2019, 1:20:59 PM2/5/19
to Burak Serdar, matteo....@gmail.com, golang-nuts
Then you want inheritance not encapsulation.

Burak Serdar

unread,
Feb 5, 2019, 1:27:00 PM2/5/19
to Robert Engels, matteo....@gmail.com, golang-nuts
On Tue, Feb 5, 2019 at 11:20 AM Robert Engels <ren...@ix.netcom.com> wrote:
>
> Then you want inheritance not encapsulation.

No, this is done at runtime, not at compile time.

Robert Engels

unread,
Feb 5, 2019, 1:27:23 PM2/5/19
to Burak Serdar, matteo....@gmail.com, golang-nuts
But even then, exposing internal details outward leads to lots of problems, testability for certain. There’s almost always a better way then GetNested, GetWrapped, or any of its derivatives.

Burak Serdar

unread,
Feb 5, 2019, 1:34:09 PM2/5/19
to Robert Engels, matteo....@gmail.com, golang-nuts
On Tue, Feb 5, 2019 at 11:27 AM Robert Engels <ren...@ix.netcom.com> wrote:
>
> But even then, exposing internal details outward leads to lots of problems, testability for certain. There’s almost always a better way then GetNested, GetWrapped, or any of its derivatives.

How so? How would you deal with something like error wrapping, for
instance? Or in this specific case, how would you get the length of
the underlying buffer if you wrap the reader with a NopCloser?

Robert Engels

unread,
Feb 5, 2019, 2:22:15 PM2/5/19
to Burak Serdar, matteo....@gmail.com, golang-nuts
As far as the error handling, this is kind of a limitation of the error interface in Go and should be solved there. But I would start with asking, why do you need the causation error? If it is increased logging, then the higher level error should be able to represent the additional detail when it is logged. If it is for control flow, then the containing error should adequately represent the error.

If you are exposing the length of the underlying buffer higher levels, that is a design problem. In the Nopcloser you are exposing the reader the caller already has access to by way of the Reader interface. The Closer is a decorator. You don’t have access the possibly real Closer in the original implementation. There is no GetWrappedCloser.

I would also argue that Nopcloser is a poor design. You are passing a object around where the Close that the object is exposing is no longer called. This can lead to very brittle code and obscure bugs - the object is no longer behaving the way the designer expected. The Closer interface expects that the resource will be closed, this is now broken.

Much of a Gos dynamic interfaces though work poorly this way... but that is another discussion.

Burak Serdar

unread,
Feb 5, 2019, 2:37:50 PM2/5/19
to Robert Engels, matteo....@gmail.com, golang-nuts
On Tue, Feb 5, 2019 at 12:22 PM Robert Engels <ren...@ix.netcom.com> wrote:
>
> As far as the error handling, this is kind of a limitation of the error interface in Go and should be solved there. But I would start with asking, why do you need the causation error? If it is increased logging, then the higher level error should be able to represent the additional detail when it is logged. If it is for control flow, then the containing error should adequately represent the error.

That's not always possible if the containing error doesn't really know
what it is wrapping. The nested error could be of different types that
the wrapper don't know about.

>
> If you are exposing the length of the underlying buffer higher levels, that is a design problem. In the Nopcloser you are exposing the reader the caller already has access to by way of the Reader interface. The Closer is a decorator. You don’t have access the possibly real Closer in the original implementation. There is no GetWrappedCloser.

Note that the inability to expose the underlying buffer length is the
problem here. The only safe way to get the length of the buffer, in
this case, is to copy it.

>
> I would also argue that Nopcloser is a poor design. You are passing a object around where the Close that the object is exposing is no longer called. This can lead to very brittle code and obscure bugs - the object is no longer behaving the way the designer expected. The Closer interface expects that the resource will be closed, this is now broken.

You are approaching this as an OO-purist. In my opinion, NopCloser is
a simple adapter that wraps readers without a close(). If it wasn't in
the library, it would've been rewritten in many projects.

Robert Engels

unread,
Feb 5, 2019, 3:46:38 PM2/5/19
to Burak Serdar, matteo....@gmail.com, golang-nuts
If the error doesn’t know what it is wrapping but the caller needs to know the wrapped error you have a design problem. It breaks all sorts of ‘isa’ semantics.

If you need to know the internal buffer length, you have a design problem. What if the wrapped buffer was unlimited or dynamic - there is no length to return - you must read until eof.

It is not a matter or OO purity, it is a simple matter of encapsulation, and not requiring upper layers to know and understand - or even have access to - internal details.

Burak Serdar

unread,
Feb 5, 2019, 3:53:06 PM2/5/19
to Robert Engels, matteo....@gmail.com, golang-nuts
On Tue, Feb 5, 2019 at 1:46 PM Robert Engels <ren...@ix.netcom.com> wrote:
>
> If the error doesn’t know what it is wrapping but the caller needs to know the wrapped error you have a design problem. It breaks all sorts of ‘isa’ semantics.

This happens if you're using multiple layers of libraries that don't
know about each other. It is not a design problem.

>
> If you need to know the internal buffer length, you have a design problem. What if the wrapped buffer was unlimited or dynamic - there is no length to return - you must read until eof.

Sometimes you need to know the internal buffer length if you're
dealing with a service behind a peculiar http front-end, like some
akamai services. You have to send the content length, and if you know
it already, you should be able to get it.

>
> It is not a matter or OO purity, it is a simple matter of encapsulation, and not requiring upper layers to know and understand - or even have access to - internal details.

Encapsulation is useful if you're hiding internal details. But some
internal details are not so internal under different circumstances,
like the length of a buffer.

Robert Engels

unread,
Feb 5, 2019, 4:10:56 PM2/5/19
to Burak Serdar, matteo....@gmail.com, golang-nuts
I think we’ll just agree to disagree.

Dan Kortschak

unread,
Feb 5, 2019, 7:18:37 PM2/5/19
to matteo....@gmail.com, golang-nuts
Personally, I think this is a bug in the behaviour of NewRequest. See h
ttps://github.com/golang/go/issues/18117 for some additional context.

Burak Serdar

unread,
Feb 5, 2019, 7:54:23 PM2/5/19
to Dan Kortschak, matteo....@gmail.com, golang-nuts
On Tue, Feb 5, 2019 at 5:18 PM Dan Kortschak <d...@kortschak.io> wrote:
>
> Personally, I think this is a bug in the behaviour of NewRequest. See h
> ttps://github.com/golang/go/issues/18117 for some additional context.

Agreed. One solution could be to have:

type HasLen interface {
int Len()
}

Then have NopCloser return a nopCloser with len if the underlying
implementation has len, with the obvious changes to NewRequest.Ugly,
but can be done without API changes.



>
> On Tue, 2019-02-05 at 05:18 -0800, matteo....@gmail.com wrote:
> > I've the following situation:
> > I proxy a request to another server and when I made a POST and create
> > a new
> > request, the contentLength is zero:
> >
> > req2, _ := http.NewRequest(req.Method, newApiUrl , req.Body)
> > fmt.Println("New request from body:", req2.ContentLength) //
> > print 0
> >
> > Checking in the source code of the NewRequest func Body don't respect
> > some
> > interface and populate the ContentLength field.
> >
> > Could be a bug? Which could be a valid approach in order to create a
> > new
> > request from an existing one and correct set the Body length?
> >
> > A working example here:
> >
> > https://play.golang.org/p/SvCDLj0NrXb
> >
> > Thanks!
> >
>

Robert Engels

unread,
Feb 5, 2019, 9:00:49 PM2/5/19
to Burak Serdar, Dan Kortschak, matteo....@gmail.com, golang-nuts
Shouldn’t you just be taking the content length from the header if forwarding the same body. There is no need for the length of the body.

Burak Serdar

unread,
Feb 5, 2019, 10:01:18 PM2/5/19
to Robert Engels, Dan Kortschak, Matteo Biagetti, golang-nuts
On Tue, Feb 5, 2019 at 7:00 PM Robert Engels <ren...@ix.netcom.com> wrote:
>
> Shouldn’t you just be taking the content length from the header if forwarding the same body. There is no need for the length of the body.

True. What I was suggesting is a fix for the general case.

robert engels

unread,
Feb 5, 2019, 10:13:45 PM2/5/19
to Burak Serdar, Dan Kortschak, Matteo Biagetti, golang-nuts
That’s what I was trying to point out. Your design is not correct. The Body is a Reader, not a Buffer - the length of the request/body may be indeterminate - that is, a stream. Attempting to get the length of an underlying buffer is not only probably not possible, but not correct in many situations.

There is a reason the Body is a ReaderCloser and not a buffer. It is part of the http specification.

Burak Serdar

unread,
Feb 6, 2019, 1:28:54 AM2/6/19
to robert engels, Dan Kortschak, Matteo Biagetti, golang-nuts
On Tue, Feb 5, 2019 at 8:13 PM robert engels <ren...@ix.netcom.com> wrote:
>
> That’s what I was trying to point out. Your design is not correct. The Body is a Reader, not a Buffer - the length of the request/body may be indeterminate - that is, a stream. Attempting to get the length of an underlying buffer is not only probably not possible, but not correct in many situations.

The length of the body *may* be indeterminate, and if that's the case,
the underlying Reader will not have a Len method. The design is to
handle the case where the underlying Reader is a Buffer with a Len
method. If the Reader has Len, then the NopCloser derived from that
will also have a Len, and NewRequest can set the content length. If
the Reader does not have Len, then the content length is unknown.

Matteo Biagetti

unread,
Feb 6, 2019, 3:22:37 AM2/6/19
to golang-nuts
Make sense, thanks for explanation

Robert Engels

unread,
Feb 6, 2019, 7:15:27 AM2/6/19
to Matteo Biagetti, golang-nuts
I see now, but if that can be the case, shouldn’t the Body be documented that the Reader may be a ReaderWithLen, and the consumer is free to type check/cast? If not, you are using internal details that you should not be.

This is a problem with Go in general. Because the returned object “implements” some interface because it happens to have the required method, doesn’t mean it was designed to be used that way, or that it has the required semantics - unless documented to have them. 

Burak Serdar

unread,
Feb 6, 2019, 9:30:55 AM2/6/19
to Robert Engels, Matteo Biagetti, golang-nuts
On Wed, Feb 6, 2019 at 5:15 AM Robert Engels <ren...@ix.netcom.com> wrote:
>
> I see now, but if that can be the case, shouldn’t the Body be documented that the Reader may be a ReaderWithLen, and the consumer is free to type check/cast? If not, you are using internal details that you should not be.

Yes, the documentation should say if the reader has a Len() method it
would be used to set the ContentLength. Len is no longer an internal
detail then.

>
> This is a problem with Go in general. Because the returned object “implements” some interface because it happens to have the required method, doesn’t mean it was designed to be used that way, or that it has the required semantics - unless documented to have them.

I agree with you there. Len() is straight forward, but in general just
because a function is named something doesn't mean it'll do the same
thing for all implementations. On the other end of the spectrum is
Java-like interfaces where you want explicit inheritance of a specific
interface. I don't know if there's anything in between, but I like
Go's approach much better.

Robert Engels

unread,
Feb 6, 2019, 9:56:50 AM2/6/19
to Burak Serdar, Matteo Biagetti, golang-nuts
But is it really? If you read the description for Len() on bytes.Buffer it is the length of unread portion. But that doesn’t mean the buffer isn’t just a portion of the entire body - it can be a chunk which is continually reloaded.

This is the danger in using private APIs publically based upon the existence of a method - it leads to very brittle code - and there are almost certainly better ways to design it to avoid these issues. If the core api is not expressive enough then it will be more difficult.

Burak Serdar

unread,
Feb 6, 2019, 10:05:48 AM2/6/19
to Robert Engels, Matteo Biagetti, golang-nuts
On Wed, Feb 6, 2019 at 7:56 AM Robert Engels <ren...@ix.netcom.com> wrote:
>
> But is it really? If you read the description for Len() on bytes.Buffer it is the length of unread portion. But that doesn’t mean the buffer isn’t just a portion of the entire body - it can be a chunk which is continually reloaded.

Reader.Len() should give how much you can read from the reader, so
for a buffer, it should return the unread portion. If you have a
reader that loads its chunks as needed and it knows the total length,
that interpretation of Len() is still correct. If you have a Reader
that doesn't know the length, then it should not implement Len().

>
> This is the danger in using private APIs publically based upon the existence of a method - it leads to very brittle code - and there are almost certainly better ways to design it to avoid these issues. If the core api is not expressive enough then it will be more difficult.

Len() is not a private API, it has fairly well-understood and agreed
upon semantics, so I think for this case it is safe to use it without
much negative effect. The http.Request could use a redesign though.

Robert Engels

unread,
Feb 6, 2019, 10:20:10 AM2/6/19
to Burak Serdar, Matteo Biagetti, golang-nuts
There is no Len() defined for io.Reader. That is the crux of the issue. You are defining it because you need it. It needs to be defined and documented by the api specification, otherwise it is an incorrect assumption and usage - aka brittle code.

Dan Kortschak

unread,
Feb 6, 2019, 4:37:43 PM2/6/19
to Robert Engels, Burak Serdar, Matteo Biagetti, golang-nuts
The generalised semantics of Len are that it returns the number of
available elements in the collection, being a cognate of the len built-
in. This means that as you consume elements from a buffer, the Len
value reduces. This is directly equivalent to

for len(buf) != 0 {
println(buf[0])
buf = buf[1:]
> > > On Feb 6, 2019, at 2:22 AM, Matteo Biagetti <matteo.biagetti@gmai

robert engels

unread,
Feb 6, 2019, 4:51:12 PM2/6/19
to Dan Kortschak, Burak Serdar, Matteo Biagetti, golang-nuts
I am not sure what that has to do with the discussion. My point was that for it to be applicable here, it needs to be defined as part of io.Reader, since that is what Body is declared as. It is not, so using in the manner outlined is not correct IMO.

Burak Serdar

unread,
Feb 6, 2019, 6:06:32 PM2/6/19
to robert engels, Dan Kortschak, Matteo Biagetti, golang-nuts
On Wed, Feb 6, 2019 at 2:50 PM robert engels <ren...@ix.netcom.com> wrote:
>
> I am not sure what that has to do with the discussion. My point was that for it to be applicable here, it needs to be defined as part of io.Reader, since that is what Body is declared as. It is not, so using in the manner outlined is not correct IMO.

Len() cannot be part of Reader. Not all readers can support it. Len()
can be a part of the implementation that can be set as Body.
Currently, the http implementation is checking for known Reader
implementations to figure out the length of data. That's a hack.
Checking if the reader supports Len() would fix that without any API
changes.

robert engels

unread,
Feb 6, 2019, 7:03:05 PM2/6/19
to Burak Serdar, Dan Kortschak, Matteo Biagetti, golang-nuts
The reason it checks for known implementations is because it “knows the implementation”, so it can be sure it has the correct semantics.

This is the problem with Go “duck typing”.

The real solution would be to declare a new interface called BodyContent, that had methods of io.Reader, and a method ContentLength() -which could be delegated to Len() - not re-use a generic method like Len(). but even then that is not foolproof because of the “duck typing”.

I know most do not agree, and see the Go “duck typing” as a plus, but IMO it is a huge problem for larger code sets with proper maintainable designs.

iv...@ibrt.me

unread,
Feb 6, 2019, 7:30:15 PM2/6/19
to golang-nuts
What you are seeing here is an edge case caused by the fact that the first NewRequest wraps your strings.Reader into a ioutil.NopCloser. When NewRequest is called the second time, the type switch no longer matches a type for which the length is known, so ContentLength is left to 0, which according to the docs means unknown. NewRequest implements this "best effort" method of determining the content length to avoid using the less efficient chunked encoding transfer method unless necessary, so you don't need to explicitly set the Content-Length header every time you create a new request.

That said, this is an artificial example which doesn't really reflect the way this API is used. If you were reverse proxying an incoming HTTP request, you would know the length of the body before reading it only if the client set a Content-Length header, which is optional. To properly proxy the request you would pass through the body reader, and copy the Content-Length header to the upstream request. You can check out the source code httputil.ReverseProxy for a working implementation. An unset Request.ContentLength does not affect the ability to actually perform the request, although it might cause it to be sent with the somewhat less efficient chunked encoding method.

Hope this helps

Dan Kortschak

unread,
Feb 7, 2019, 2:06:22 AM2/7/19
to robert engels, Burak Serdar, Matteo Biagetti, golang-nuts
Addressing the first sentence, it was a direct answer to a comment you
made:

> But is it really? If you read the description for Len() on
> bytes.Buffer it is the length of unread portion. But that doesn’t
> mean the buffer isn’t just a portion of the entire body - it can be a
> chunk which is continually reloaded.

As far as the claim that there is a need to have a Len method in
io.Reader, have a look at the code in question. It type asserts on
three concrete types that are known to the function, all three have a
Len method and this is used to obtain the known length. All other
io.Readers are considered to have an unknown length.

Whether it's wrong to use Len depends on whether there is a generally
accepted and consistent set of semantics to Len() int. There is. This
is strengthened if the use of an existing Len method is noted in the
docs.

Robert Engels

unread,
Feb 7, 2019, 8:07:58 AM2/7/19
to Dan Kortschak, Burak Serdar, Matteo Biagetti, golang-nuts
You are agreeing with me. A type switch on concrete types (that you control) is far different than using an available Len() method and assuming the same semantics.

Dan Kortschak

unread,
Feb 7, 2019, 2:08:29 PM2/7/19
to Robert Engels, Burak Serdar, Matteo Biagetti, golang-nuts
Yeah, I'm not agreeing with you.
> > > > > > > On Wed, Feb 6, 2019 at 5:15 AM Robert Engels <rengels@ix.

robert engels

unread,
Feb 7, 2019, 2:27:40 PM2/7/19
to Dan Kortschak, Burak Serdar, Matteo Biagetti, golang-nuts
I am not following. You stated that the usage of Len was internal and a type switch on known concrete types, so how is related to how the OP was attempting to have things work?

There is no “generally accepted use of Len()”, otherwise it would not need to perform a type switch on known concrete types - it would cast to an interface declaring Len(), and use the interface, and then it would work with any type.

Dan Kortschak

unread,
Feb 7, 2019, 2:56:39 PM2/7/19
to robert engels, Burak Serdar, Matteo Biagetti, golang-nuts
I didn't mention the word internal, nor did I imply it; with
documentation stating that it would be used, it is clearly *not*
internal.

If you look at the code in question, you can see a probable reason why
a Lener interface is not used; for each of the blessed types, a
concrete copy of the pointed-to-value is made to allow GetBody to
return it. This cannot be done with an interface value without the use
of reflect.

Please show me a Len method in the standard library that does not
return the number of available-to-access elements in a collection.
> > > > > > > > On Feb 6, 2019, at 8:30 AM, Burak Serdar <bserdar@ieee.
> > > > > > > > > > > it, send an email to golang-nuts...@googlegroups.
> > > > > > > > > > > com.
> > > > > > > > > > > For more options, visit https://groups.google.com
> > > > > > > > > > > /d/o
> > > > > > > > > > > ptou
> > > > > > > > > > > t.
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > --
> > > > > > > > > > > You received this message because you are
> > > > > > > > > > > subscribed
> > > > > > > > > > > to
> > > > > > > > > > > the
> > > > > > > > > > > Google Groups "golang-nuts" group.
> > > > > > > > > > > To unsubscribe from this group and stop receiving
> > > > > > > > > > > emails
> > > > > > > > > > > from
> > > > > > > > > > > it, send an email to golang-nuts...@googlegroups.
> > > > > > > > > > > com.
> > > > > > > > > > > For more options, visit https://groups.google.com
> > > > > > > > > > > /d/o
> > > > > > > > > > > ptou
> > > > > > > > > > > t.
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > --
> > > > > > > > > > > You received this message because you are
> > > > > > > > > > > subscribed
> > > > > > > > > > > to
> > > > > > > > > > > the
> > > > > > > > > > > Google Groups "golang-nuts" group.
> > > > > > > > > > > To unsubscribe from this group and stop receiving
> > > > > > > > > > > emails
> > > > > > > > > > > from
> > > > > > > > > > > it, send an email to golang-nuts...@googlegroups.
> > > > > > > > > > > com.
> > > > > > > > > > > For more options, visit https://groups.google.com
> > > > > > > > > > > /d/o
> > > > > > > > > > > ptou
> > > > > > > > > > > t.
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > --
> > > > > > > > > > > You received this message because you are
> > > > > > > > > > > subscribed
> > > > > > > > > > > to
> > > > > > > > > > > the
> > > > > > > > > > > Google Groups "golang-nuts" group.
> > > > > > > > > > > To unsubscribe from this group and stop receiving
> > > > > > > > > > > emails
> > > > > > > > > > > from
> > > > > > > > > > > it, send an email to golang-nuts...@googlegroups.
> > > > > > > > > > > com.
> > > > > > > > > > > For more options, visit https://groups.google.com
> > > > > > > > > > > /d/o
> > > > > > > > > > > ptou
> > > > > > > > > > > t.
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > --
> > > > > > > > > You received this message because you are subscribed
> > > > > > > > > to
> > > > > > > > > the
> > > > > > > > > Google Groups "golang-nuts" group.
> > > > > > > > > To unsubscribe from this group and stop receiving
> > > > > > > > > emails
> > > > > > > > > from
> > > > > > > > > it,
> > > > > > > > > send an email to golang-nuts+unsubscribe@googlegroups
> > > > > > > > > .com
> > > > > > > > > .
> > > > > > > > > For more options, visit https://groups.google.com/d/o
> > > > > > > > > ptou
> > > > > > > > > t.
> > > > > > > > >
> > > > > > > > > --
> > > > > > > > > You received this message because you are subscribed
> > > > > > > > > to
> > > > > > > > > the
> > > > > > > > > Google Groups "golang-nuts" group.
> > > > > > > > > To unsubscribe from this group and stop receiving
> > > > > > > > > emails
> > > > > > > > > from
> > > > > > > > > it,
> > > > > > > > > send an email to golang-nuts+unsubscribe@googlegroups

robert engels

unread,
Feb 7, 2019, 3:55:57 PM2/7/19
to Dan Kortschak, Burak Serdar, Matteo Biagetti, golang-nuts
I see the documented use of the types in NewRequest - you are correct - I was wrong.

But, it could of easily also declared that if the provided Reader is also a Lener, it uses it to determine the content length. Why have this behavior for Closer and not for Lener? Then you don’t need the type switch. You say, well the copy...

The current code with the copy is broken - the caller could continue to modify the contents of the bytes.Buffer and things would not work as expected since the backing array of the slice is shared - so how is the copy helping ? The length will remain the same, but the data represented could be corrupted.

The correct solution is to declare that NewRequest takes an interface Content, that has both Reader and ContentLength methods, where ContentLength() can return -1 if the content length is indeterminate. Then declare simple facades for Content from bytes.Buffer, a string, etc. And also declare that continued use of the Content after NewRequest is undefined. And if you wanted to retain the simplicity, just declare it uses ContentLength like it uses Closer.

I am all for the simplicity of Go, but “solutions" like NewRequest are not the way modern software should be developed. Casting an interface to a concrete type is a sign of code that needs design work. Having to read the doc in addition to the method signature is also a sign the interface needs work (primarily since changes to the doc can/will/might change behavior but it avoids compile time type checking = brittle code with obscure bugs).

robert engels

unread,
Feb 7, 2019, 4:01:48 PM2/7/19
to Dan Kortschak, Burak Serdar, Matteo Biagetti, golang-nuts
And to come full circle, this poorly declared method, with hidden internal implementation details, is exactly the cause of OP’s initial problem. Not expecting that the Body passed in, and retrieved later could be used as a Body of a new request - why wouldn’t he think that ?

Dan Kortschak

unread,
Feb 7, 2019, 4:06:29 PM2/7/19
to robert engels, Burak Serdar, Matteo Biagetti, golang-nuts
Their is an assumption in the code that users don't mutate values under
the feet of routines that have been called. The copy does not protect
against that and is not designed for that purpose; it is there to make
the GetBody function return a zeroed copy of the body (for some
definition of zeroed - zero is the state that was handed in to the
NewRequest call). If the behaviour of the copy here is broken, any Go
code that causes a slice, map or pointer to be retained is broken since
the user can mutate the backing data after the call has done work on
it. This becomes a philosophical point.

There is a simple work around (not as simple as having NewRequest Just
Do The Right Thing™, but reasonable) that does not require reflect and
puts the control in the users' hands. Since the ContentLength and
GetBody fields are exported, they can be set after the return of
NewRequest in the same way that NewRequest does for the blessed types,
but with the users' knowledge of internal behaviours of their types.

An example of this is here https://bitbucket.org/ausocean/iot/pull-requ
ests/42
> > > > > > > > On Feb 6, 2019, at 3:37 PM, Dan Kortschak <dan@kortscha
> > > > > > > > > > > send an email to golang-nuts+unsubscribe@googlegr
> > > > > > > > > > > oups
> > > > > > > > > > > .com
> > > > > > > > > > > .
> > > > > > > > > > > For more options, visit https://groups.google.com
> > > > > > > > > > > /d/o
> > > > > > > > > > > ptou
> > > > > > > > > > > t.
> > > > > > > > > > >
> > > > > > > > > > > --
> > > > > > > > > > > You received this message because you are
> > > > > > > > > > > subscribed
> > > > > > > > > > > to
> > > > > > > > > > > the
> > > > > > > > > > > Google Groups "golang-nuts" group.
> > > > > > > > > > > To unsubscribe from this group and stop receiving
> > > > > > > > > > > emails
> > > > > > > > > > > from
> > > > > > > > > > > it,
> > > > > > > > > > > send an email to golang-nuts+unsubscribe@googlegr

robert engels

unread,
Feb 7, 2019, 4:33:46 PM2/7/19
to Dan Kortschak, Burak Serdar, Matteo Biagetti, golang-nuts
I agree with you on the correct solution - vs. the OP’s request of the GetWrappedXXXX method.

I guess I still don’t understand the “zeroed” concern though. If you adhere to the “don’t mutate values…” then why do the zero copy at all ? The state of the body should still be the state it was passed in as (unless the caller was breaking the aforementioned rule anyway…).

Having “blessed types” is not a good design IMO - better to just declare the required interfaces and use those - especially when the object being provided as a parameter is already an interface… just seems lazy… (and long-term error prone).

Dan Kortschak

unread,
Feb 7, 2019, 4:43:17 PM2/7/19
to robert engels, Burak Serdar, Matteo Biagetti, golang-nuts
Keeping a zeroed state is important for the GetBody func because the
io.ReadCloser returned by GetBody can be read and closed. If there is
no zero state for the next time GetBody is called, it is not
idempotent. This would break the whole point of it existing.

See https://go-review.googlesource.com/c/go/+/31733/
> > > > > > > > On Feb 7, 2019, at 1:05 AM, Dan Kortschak <dan@kortscha
> > > > > > > > > > > > > send an email to golang-nuts+unsubscribe@goog
> > > > > > > > > > > > > legr
> > > > > > > > > > > > > oups
> > > > > > > > > > > > > .com
> > > > > > > > > > > > > .
> > > > > > > > > > > > > For more options, visit https://groups.google
> > > > > > > > > > > > > .com
> > > > > > > > > > > > > /d/o
> > > > > > > > > > > > > ptou
> > > > > > > > > > > > > t.
> > > > > > > > > > > > >
> > > > > > > > > > > > > --
> > > > > > > > > > > > > You received this message because you are
> > > > > > > > > > > > > subscribed
> > > > > > > > > > > > > to
> > > > > > > > > > > > > the
> > > > > > > > > > > > > Google Groups "golang-nuts" group.
> > > > > > > > > > > > > To unsubscribe from this group and stop
> > > > > > > > > > > > > receiving
> > > > > > > > > > > > > emails
> > > > > > > > > > > > > from
> > > > > > > > > > > > > it,
> > > > > > > > > > > > > send an email to golang-nuts+unsubscribe@goog
> > > > > > > > > > > > > legr
> > > > > > > > > > > > > oups
> > > > > > > > > > > > > .com
> > > > > > > > > > > > > .

robert engels

unread,
Feb 7, 2019, 5:11:02 PM2/7/19
to Dan Kortschak, Burak Serdar, Matteo Biagetti, golang-nuts
So GetBody just fails… It returns NoBody in this case.. which means calling code will just break (when the original request is not one of the known types).

So, according to the referenced issue, 307/308 redirects won’t work when the underlying request is not a known type.

This is a very brittle API IMO. If you need to do stuff like this, the readers should be using mark/reset etc with buffered streams, not relying on special cased functions like GetBody that don’t work in all cases.

Dan Kortschak

unread,
Feb 7, 2019, 5:31:29 PM2/7/19
to robert engels, Burak Serdar, Matteo Biagetti, golang-nuts
I agree that this is a brittle API and it's one that has bitten us
twice (partly because - I beleive - the dev_appserver.py development
server is broken in how it deals with the cases here. This is why I
filed an issue - though not with great hope of a fix other than maybe
improving the documentation.
> > > > > > > > On Feb 7, 2019, at 1:07 PM, Dan Kortschak <dan@kortscha
> > > > > > > > > > > > > > > send an email to golang-nuts+unsubscribe@
> > > > > > > > > > > > > > > goog
> > > > > > > > > > > > > > > legr
> > > > > > > > > > > > > > > oups
> > > > > > > > > > > > > > > .com
> > > > > > > > > > > > > > > .
> > > > > > > > > > > > > > > For more options, visit https://groups.go
> > > > > > > > > > > > > > > ogle
> > > > > > > > > > > > > > > .com
> > > > > > > > > > > > > > > /d/o
> > > > > > > > > > > > > > > ptou
> > > > > > > > > > > > > > > t.
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > --
> > > > > > > > > > > > > > > You received this message because you are
> > > > > > > > > > > > > > > subscribed
> > > > > > > > > > > > > > > to
> > > > > > > > > > > > > > > the
> > > > > > > > > > > > > > > Google Groups "golang-nuts" group.
> > > > > > > > > > > > > > > To unsubscribe from this group and stop
> > > > > > > > > > > > > > > receiving
> > > > > > > > > > > > > > > emails
> > > > > > > > > > > > > > > from
> > > > > > > > > > > > > > > it,
> > > > > > > > > > > > > > > send an email to golang-nuts+unsubscribe@
> > > > > > > > > > > > > > > goog
> > > > > > > > > > > > > > > legr
> > > > > > > > > > > > > > > oups
> > > > > > > > > > > > > > > .com
> > > > > > > > > > > > > > > .
Reply all
Reply to author
Forward
0 new messages