Chunked transfer encoding

[rampage644]

I've faced an issue similiar to what is described in #391. Namely, other 3rd party RESTful service is unable to parse incoming JSON body.

In the end, it boiled down to the inability of existing nginx/uwsgi/django setup to properly parse HTTP request without Content-Length header (dunno what element in the chain should be responsible for this).

What was happening under the hood is the way go-kit creates http.Request and initializes its body: since body is nil during construction, r.ContentLength is not set. The solution (as in mentioned issue) is to redefine encodeJSONRequest function to properly set required parameter.

Here we create a request with nil body: https://github.com/go-kit/kit/blob/master/transport/http/client.go#L114. This code is skipped due to nil body: https://github.com/golang/go/blob/master/src/net/http/request.go#L827

Please advise what is the best way of dealing with this issue.

[rampage644]

This is how my encode functions looks like:

func encodeJSONRequest(c context.Context, r *http.Request, request interface{}) error {

       // omitting other stuff from original function

	var b bytes.Buffer
	err := json.NewEncoder(&b).Encode(request)
	r.Body = ioutil.NopCloser(&b)
	// We set content length explicitly here since go-kit constructs request
	// with nil body and thus failing to initialize some fields.
	// We can omit setting it but this results in using chunked transfer-encoding
	// that cause problems on the nginx/uwsgi/django side - it just can't
	// deal with no Content-Length header set.
	r.ContentLength = int64(b.Len())
	return err
}

[peterbourgon]

That seems correct to me. Is there something unsatisfying about this?

[rampage644]

I thought if it's worth hiding this in the go-kit default encoding function. Or maybe change the way http request is created to allow it to be fully initialized.

[rampage644]

Right now I feel this leaks low-level transport details into encoder function - "should I set only the body here or I need to figure out what is expected by the underlying low-level things". Strictly speaking, original code also initializes GetBody function as well, and it could be confusing just to duplicate code from src/net/http/request.
Additionally, http request code might change evolve.

[peterbourgon]

What alternative implementation possibly exists?

[LasTshaMAN]

@peterbourgon

As I currently understand the problem, go-kit tries to hide some boilerplate details about *http.Request creation from the user, yet it doesn't fully succeed in the task,

as the result, we get hard-to-debug behavior ...

I see two possible solutions (after some brief investigation), both requiring changing the signature of Encode function:

• let the user be fully responsible for *http.Request:

type EncodeRequestFunc func(context.Context, interface{}) (*http.Request, error)

• force the user to return Content-Length explicitly, so that go-kit will have enough information about the request to be able to set Content-Length properly:

type EncodeRequestFunc func(context.Context, *http.Request, interface{}) (int64, error)

[peterbourgon]

I'm open to the first option, and would happily take a PR as long as it doesn't change the existing API in backwards-incompatible ways.

[LasTshaMAN]

@peterbourgon, I've created a PR implementing the first option - #916 - let's see whether you like it (it doesn't break the existing API).

[peterbourgon]

I've done an alternative implementation to #916 in #971. Can you confirm this will solve the issue? @rampage644 @LasTshaMAN

[LasTshaMAN]

#971 is semantically equivalent fix as far as I can tell, so, feel free to close #916 once you've merged #971,

I'm happy it is gonna be solved now!