Skip to content
Permalink
Browse files

Always be explicit about HTTP request body lengths when possible.

This works around golang/go#17071 in particular, and is more robust to
future similar issues.

The sniffing magic performed by http.NewRequest has now been hoisted to
the one place where it's actually needed.
  • Loading branch information...
jacobsa committed Sep 14, 2016
1 parent 8b0ee21 commit 17f7008e69293a57067b3e9c0b7679ccae88bef1
Showing with 62 additions and 41 deletions.
  1. +3 −3 gcs/bucket.go
  2. +4 −7 gcs/compose_objects.go
  3. +1 −6 gcs/copy_object.go
  4. +20 −7 gcs/create_object.go
  5. +1 −1 gcs/read.go
  6. +17 −5 gcs/update_object.go
  7. +16 −12 httputil/request.go
@@ -168,7 +168,7 @@ func (b *bucket) ListObjects(
}

// Create an HTTP request.
httpReq, err := httputil.NewRequest(ctx, "GET", url, nil, b.userAgent)
httpReq, err := httputil.NewRequest(ctx, "GET", url, nil, 0, b.userAgent)
if err != nil {
err = fmt.Errorf("httputil.NewRequest: %v", err)
return
@@ -221,7 +221,7 @@ func (b *bucket) StatObject(
}

// Create an HTTP request.
httpReq, err := httputil.NewRequest(ctx, "GET", url, nil, b.userAgent)
httpReq, err := httputil.NewRequest(ctx, "GET", url, nil, 0, b.userAgent)
if err != nil {
err = fmt.Errorf("httputil.NewRequest: %v", err)
return
@@ -291,7 +291,7 @@ func (b *bucket) DeleteObject(
}

// Create an HTTP request.
httpReq, err := httputil.NewRequest(ctx, "DELETE", url, nil, b.userAgent)
httpReq, err := httputil.NewRequest(ctx, "DELETE", url, nil, 0, b.userAgent)
if err != nil {
err = fmt.Errorf("httputil.NewRequest: %v", err)
return
@@ -19,7 +19,6 @@ import (
"encoding/json"
"errors"
"fmt"
"io"
"io/ioutil"
"net/http"
"net/url"
@@ -33,7 +32,7 @@ import (
)

func (b *bucket) makeComposeObjectsBody(
req *ComposeObjectsRequest) (rc io.ReadCloser, err error) {
req *ComposeObjectsRequest) (body []byte, err error) {
// Create a request in the form expected by the API.
r := storagev1.ComposeRequest{
Destination: &storagev1.Object{
@@ -53,15 +52,12 @@ func (b *bucket) makeComposeObjectsBody(
}

// Serialize it.
j, err := json.Marshal(&r)
body, err = json.Marshal(&r)
if err != nil {
err = fmt.Errorf("json.Marshal: %v", err)
return
}

// Create a ReadCloser.
rc = ioutil.NopCloser(bytes.NewReader(j))

return
}

@@ -116,7 +112,8 @@ func (b *bucket) ComposeObjects(
ctx,
"POST",
url,
body,
ioutil.NopCloser(bytes.NewReader(body)),
int64(len(body)),
b.userAgent)

if err != nil {
@@ -18,10 +18,8 @@ import (
"encoding/json"
"errors"
"fmt"
"io/ioutil"
"net/http"
"net/url"
"strings"
"unicode/utf8"

"github.com/jacobsa/gcloud/httputil"
@@ -70,11 +68,8 @@ func (b *bucket) CopyObject(
RawQuery: query.Encode(),
}

// We don't want to update any metadata.
body := ioutil.NopCloser(strings.NewReader(""))

// Create an HTTP request.
httpReq, err := httputil.NewRequest(ctx, "POST", url, body, b.userAgent)
httpReq, err := httputil.NewRequest(ctx, "POST", url, nil, 0, b.userAgent)
if err != nil {
err = fmt.Errorf("httputil.NewRequest: %v", err)
return
@@ -19,10 +19,10 @@ import (
"encoding/json"
"errors"
"fmt"
"io"
"io/ioutil"
"net/http"
"net/url"
"strings"
"unicode/utf8"

"github.com/jacobsa/gcloud/httputil"
@@ -33,7 +33,7 @@ import (

// Create the JSON for an "object resource", for use as an Objects.insert body.
func (b *bucket) makeCreateObjectBody(
req *CreateObjectRequest) (rc io.ReadCloser, err error) {
req *CreateObjectRequest) (body []byte, err error) {
// Convert to storagev1.Object.
rawObject, err := toRawObject(b.Name(), req)
if err != nil {
@@ -42,15 +42,12 @@ func (b *bucket) makeCreateObjectBody(
}

// Serialize.
j, err := json.Marshal(rawObject)
body, err = json.Marshal(rawObject)
if err != nil {
err = fmt.Errorf("json.Marshal: %v", err)
return
}

// Create a ReadCloser.
rc = ioutil.NopCloser(bytes.NewReader(j))

return
}

@@ -105,7 +102,8 @@ func (b *bucket) startResumableUpload(
ctx,
"POST",
url,
body,
ioutil.NopCloser(bytes.NewReader(body)),
int64(len(body)),
b.userAgent)

if err != nil {
@@ -164,12 +162,27 @@ func (b *bucket) CreateObject(
return
}

// Special case: for a few common cases we can explicitly specify a body
// length, which may assist the HTTP package. In particular, it works around
// https://golang.org/issue/17071 in versions before Go 1.7.2 when the
// information is available.
contentsLength := int64(-1)
switch v := req.Contents.(type) {
case *bytes.Buffer:
contentsLength = int64(v.Len())
case *bytes.Reader:
contentsLength = int64(v.Len())
case *strings.Reader:
contentsLength = int64(v.Len())
}

// Set up a follow-up request to the upload URL.
httpReq, err := httputil.NewRequest(
ctx,
"PUT",
uploadURL,
ioutil.NopCloser(req.Contents),
contentsLength,
b.userAgent)

if err != nil {
@@ -63,7 +63,7 @@ func (b *bucket) NewReader(
}

// Create an HTTP request.
httpReq, err := httputil.NewRequest(ctx, "GET", url, nil, b.userAgent)
httpReq, err := httputil.NewRequest(ctx, "GET", url, nil, 0, b.userAgent)
if err != nil {
err = fmt.Errorf("httputil.NewRequest: %v", err)
return
@@ -15,9 +15,9 @@
package gcs

import (
"bytes"
"encoding/json"
"fmt"
"io"
"io/ioutil"
"net/http"
"net/url"
@@ -29,7 +29,7 @@ import (
)

func (b *bucket) makeUpdateObjectBody(
req *UpdateObjectRequest) (rc io.ReadCloser, err error) {
req *UpdateObjectRequest) (body []byte, err error) {
// Set up a map representing the JSON object we want to send to GCS. For now,
// we don't treat empty strings specially.
jsonMap := make(map[string]interface{})
@@ -70,8 +70,13 @@ func (b *bucket) makeUpdateObjectBody(
return
}

// Set up a ReadCloser.
rc = ioutil.NopCloser(r)
// Serialize eagerly so that we know the content length, avoiding issues in
// net/http like https://golang.org/issue/17071.
body, err = ioutil.ReadAll(r)
if err != nil {
err = fmt.Errorf("reading JSON: %v", err)
return
}

return
}
@@ -113,7 +118,14 @@ func (b *bucket) UpdateObject(
}

// Create an HTTP request.
httpReq, err := httputil.NewRequest(ctx, "PATCH", url, body, b.userAgent)
httpReq, err := httputil.NewRequest(
ctx,
"PATCH",
url,
ioutil.NopCloser(bytes.NewReader(body)),
int64(len(body)),
b.userAgent)

if err != nil {
err = fmt.Errorf("httputil.NewRequest: %v", err)
return
@@ -22,7 +22,8 @@ import (
"golang.org/x/net/context"
)

// Create an HTTP request with the supplied information.
// Create an HTTP request with the supplied information. bodyLength must be the
// total amount of data that will be returned by body, or -1 if unknown.
//
// Unlike http.NewRequest:
//
@@ -34,8 +35,9 @@ import (
// between actual slashes in the path and escaped ones (cf.
// http://goo.gl/rWX6ps).
//
// * This function doesn't magically re-interpret an io.Reader as an
// io.ReadCloser when possible.
// * This function contains less magic: it insists on an io.ReadCloser as in
// http.Request.Body, and doesn't attempt to imperfectly sniff content
// length.
//
// * This function provides a convenient choke point to ensure we don't
// forget to set a user agent header.
@@ -45,18 +47,20 @@ func NewRequest(
method string,
url *url.URL,
body io.ReadCloser,
bodyLength int64,
userAgent string) (req *http.Request, err error) {
// Create the request.
req = &http.Request{
Method: method,
URL: url,
Proto: "HTTP/1.1",
ProtoMajor: 1,
ProtoMinor: 1,
Header: make(http.Header),
Body: body,
Host: url.Host,
Cancel: ctx.Done(),
Method: method,
URL: url,
Proto: "HTTP/1.1",
ProtoMajor: 1,
ProtoMinor: 1,
Header: make(http.Header),
Body: body,
ContentLength: bodyLength,
Host: url.Host,
Cancel: ctx.Done(),
}

// Set the User-Agent header.

0 comments on commit 17f7008

Please sign in to comment.
You can’t perform that action at this time.