package main

import (
	"fmt"
	"net/http/httputil"

	"gopkg.in/h2non/gentleman.v1"
	"gopkg.in/h2non/gentleman.v1/context"
)

func main() {
	req := gentleman.NewRequest()
	req.Method("GET")
	req.URL("httpbin.org/post")
	req.BodyString("Body Hell!")
	req.UseHandler("before dial", func(ctx *context.Context, h context.Handler) {
		fmt.Printf("Before setting context:%T - %v\n", ctx.Request.Body, ctx.Request.Body)
		ctx.Set("Foo", "Bar")
		fmt.Printf("After setting context:%T - %v\n", ctx.Request.Body, ctx.Request.Body)
		httputil.DumpRequest(ctx.Request, true)
		fmt.Printf("After DumpRequest:%T - %v\n", ctx.Request.Body, ctx.Request.Body)
		h.Next(ctx)
	})
	req.Do()
}

Output:

Before setting context:*context.contextReadCloser - &{{0xc042068d80} map[$phase:before dial]}
After setting context:*context.contextReadCloser - &{{0xc042068d80} map[Foo:Bar $phase:before dial]}
After DumpRequest:ioutil.nopCloser - {Body Hell!}

With so many issues caused by wrapping context storage in http.Request.Body , I think we urgently need a solution for context storing.

I wanted to try gentleman doing a simple:

func (r *Repository) Send(authPayload, method, url string, data interface{}) ([]byte, error) {
    req := r.c.Request()
    req.AddHeader("Auth-Server-Payload", authPayload)
    req.Method(method)
    req.URL(url)
    req.JSON(data)
    res, err := req.Send()
    if err != nil {
        return nil, err
    }
        return res.Bytes(), nil
}

But when I use it, the HTTP method is overridden and the client always send a POST.

When I write fmt.Printf(req.Context.Request.Method) just before the Send and it prints a GET, I can see that a fmt.Printf(ctx.Request.Method) in the doDial method of the dispatcher prints a POST.

Am I doing something wrong ?

I am wondering if one instance of Gentleman can be shared across multiple go-routines Or every go-routine should create it's own instance every time a request comes?

I may be wrong but for now, it seems that the setting client methods such as func (c *Client) BaseURL(uri string) *Client only allows one time uses.

You can't really do:

cli := gentleman.New()
cli.BaseURL("foo.com")
cli.BaseURL("bar.com")

Because a new middleware is added each time.

The thing is I really need that kind of feature to use gentleman in a database driver because I need to be able to switch the DB url at any time, without touching the other middlewares.

I'm pretty sure there must be a workaround to do that right now. What would be the best way ?

Same code compiled with go 1.7 and 1.9 produce different requests.

On go 1.7:

DEBU[0001] http request
GET /api-v2.1/ HTTP/0.0
Host: test.sdn:8765
Content-Type: application/json
Referer: http://test.sdn:8765/api-v2.1
User-Agent: gentleman/2.0.2
X-Auth-Token: censored

Go 1.9:

DEBU[0001] http request
GET /api-v2.1/ HTTP/0.0
Host: test.sdn:8765
Content-Type: application/json
Content-Type: application/json
Referer: http://test.sdn:8765/api-v2.1
User-Agent: gentleman/2.0.2
User-Agent: gentleman/2.0.2
X-Auth-Token: censored
X-Auth-Token: censored

I workaround that by making custom plugin:

func NewRedirectPlugin() plugin.Plugin {
	return plugin.NewRequestPlugin(func(ctx *context.Context, h context.Handler) {
		ctx.Client.CheckRedirect = func(req *http.Request, pool []*http.Request) error {
			if len(pool) >= redirect.RedirectLimit {
				return redirect.ErrRedirectLimitExceeded
			}

			// redirect.Config() copy Headers from pool[0].Headers to req.Headers using req.Headers.Add()
			// That works on Go 1.7 (and seems required), but on Go 1.9 that produce header duplicates.
			// E.g. two X-Auth-Token instead of one. That broke auth.
			// Seems that in Go 1.9 net/http do headers copy

			return nil
		}
		h.Next(ctx)
	})
}

Steps to reproduce:

1. create a client
2. create a request from the client
3. register middleware for error phase for the request (not the client!)
4. trigger an error (eg: call a random url: localhost:12345)

Registered middleware won't run.

In middleware.go, Layer.Run method:

	ctx = s.parent.Run(phase, ctx)
	if ctx.Error != nil || ctx.Stopped {
		return ctx
	}

This will cause middlewares registered for error phase not to run if there is an error.

Possible solution:

	ctx = s.parent.Run(phase, ctx)
	if (phase != "error" && ctx.Error != nil) || ctx.Stopped {
		return ctx
	}

but I'm not sure what that would break.

Some of our tests are run with --race to check for race conditions and I have traced an issue with a failing test that seem to come from gentelmen's use of finalizer. There are a few uses of runtime.SetFinalizer() which I believe are incorrect. See the following code:

func SetTransportFinalizer(transport *http.Transport) {
	runtime.SetFinalizer(&transport, func(t **http.Transport) {
		(*t).CloseIdleConnections()
	})
}

You get a pointer to an http.Transport and you're setting a finalizer for the pointer to that pointer. This means that the finalizer will fire when the local variable is GCed and not when the value itself. Suggestions

• The following solves the issue for us:

func SetTransportFinalizer(transport *http.Transport) {
	runtime.SetFinalizer(transport, func(t *http.Transport) {
		t.CloseIdleConnections()
	})
}

• this probably needs to be changed also in response.EnsureResponseFinalized too

• But.. I think you should reconsider the usage of the finalizer at all. I know it as been copied from Hashicorp's go-cleanhttp package but note that they have removed that from their package.

• Also note that in the same file you have an almost identical func EnsureTransporterFinalized()which doesn't seem to be used anywhere.

We're using the gentleman client to make calls to 1st party services that can result in redirects. Initially, we were using the 'Trusted' field of the plugin to ensure that any headers were forwarded. For security purposes, we wanted to add a capability and limit header forwarding to only our 1st party services so any potential redirection to a 3rd party service wouldn't end up exposing sensitive information. In order to accomplish this, we created a fork of the redirect plugin that includes a field for a list of trusted host suffixes before doing the header copying.

It would be great to have that feature as part of the upstream gentleman, and I'd be happy to put together a PR for it if there is interest. But I wanted to open an issue and check on feasibility before just dropping a PR on your doorstep.

Thanks!

You wrap the context storage in *http.Request.Body.

When using body plugin, the plugin replace the *http.Request.Body directly.

Here is a example to reproduce the problem.

package main

import (
	"fmt"

	"gopkg.in/h2non/gentleman.v1"
)

func main() {
	req := gentleman.NewRequest()
	req.Method("GET")
	req.URL("http://httpbin.org/headers")
	// Define the URL path at request level
	//req.Path("/headers")
	// Set a new header field
	req.SetHeader("Client", "gentleman")
	req.Context.Set("Foo", "Bar")
	req.BodyString("Body Hell!")
	// Perform the request
	fmt.Println(req.Context.GetAll(), req.Context.Stopped)
	res, err := req.Do()
	fmt.Println(req.Context.GetAll(), req.Context.Stopped)
	fmt.Println(res.Context.GetAll(), res.Context.Stopped)
	if err != nil {
		fmt.Printf("Request error: %s\n", err)
		return
	}
	if !res.Ok {
		fmt.Printf("Invalid server response: %d\n", res.StatusCode)
		return
	}
	// Reads the whole body and returns it as string
	fmt.Printf("Body: %s", res.String())
}

Output:

map[Foo:Bar] false
map[$phase:response] false
map[$phase:response] false
Body: {
  "headers": {
    "Accept-Encoding": "gzip", 
    "Client": "gentleman", 
    "Content-Length": "14", 
    "Content-Type": "application/json", 
    "Host": "httpbin.org", 
    "User-Agent": "gentleman/1.0.2"
  }
}

We are using openapi-cli-generator which uses gentleman as its http client. We need to use the options method, and that is what this PR for. Thank you

I imagined that cloning a request and running its request phase would be enough to have a representation of its body. However this solution doesn't work when the request is generated using an io.Reader

If you run the tests i added the last assert fails.

When creating a new request, the GetBody property of the request is set according to the body parameter. This attribute will affect the client's behavior after receiving the 307, 308 response code. If it is empty, it is likely that it will not follow redirect.

if body != nil {
		switch v := body.(type) {
		case *bytes.Buffer:
			req.ContentLength = int64(v.Len())
			buf := v.Bytes()
			req.GetBody = func() (io.ReadCloser, error) {
				r := bytes.NewReader(buf)
				return io.NopCloser(r), nil
			}
}

if ireq.GetBody == nil && ireq.outgoingLength() != 0 {
	// We had a request body, and 307/308 require
	// re-sending it, but GetBody is not defined. So just
	// return this response to the user instead of an
	// error, like we did in Go 1.7 and earlier.
	shouldRedirect = false
}

But in gentleman the 'body.go ' and the createRequest the GetBody（） Method is not define.

// String defines the HTTP request body based on the given string.
func String(data string) p.Plugin {
	return p.NewRequestPlugin(func(ctx *c.Context, h c.Handler) {
		ctx.Request.Method = getMethod(ctx)
		ctx.Request.Body = utils.StringReader(data)
		ctx.Request.ContentLength = int64(bytes.NewBufferString(data).Len())
		h.Next(ctx)
	})
}


// JSON defines a JSON body in the outgoing request.
// Supports strings, array of bytes or buffer.
func JSON(data interface{}) p.Plugin {
	return p.NewRequestPlugin(func(ctx *c.Context, h c.Handler) {
		buf := &bytes.Buffer{}
	switch data.(type) {
	case string:
		buf.WriteString(data.(string))
	case []byte:
		buf.Write(data.([]byte))
	default:
		if err := json.NewEncoder(buf).Encode(data); err != nil {
			h.Error(ctx, err)
			return
		}
	}

	ctx.Request.Method = getMethod(ctx)
	ctx.Request.Body = ioutil.NopCloser(buf)
	ctx.Request.ContentLength = int64(buf.Len())
	ctx.Request.Header.Set("Content-Type", "application/json")
	h.Next(ctx)
})
..........
}

// createRequest creates a default http.Request instance.
func createRequest() *http.Request {
	// Create HTTP request
	req := &http.Request{
		Method:     "GET",
		URL:        &url.URL{},
		Host:       "",
		ProtoMajor: 1,
		ProtoMinor: 1,
		Proto:      "HTTP/1.1",
		Header:     make(http.Header),
		Body:       utils.NopCloser(),
	}
	// Return shallow copy of Request with the new context
	return req.WithContext(emptyContext())
}

I am creating formdata like below : Server requires form-data as : name=file ; filename= The value of name is expected to "file" on sever and filename is the actual filename Using the current mutipart I can only generate something like : 1> Content-Disposition;form-data; name=file ; filename=file 2> Content-Disposition;form-data; name=a.txt ; filename=a.txt

Cannot get something like : 3>Content-Disposition;form-data; name=file ; filename=a.txt

------WebKitFormBoundaryePkpFF7tjBAqx29L Content-Disposition: form-data; name="file"; filename="a.txt" Content-Type: application/octet**

... contents of file goes here ...

------WebKitFormBoundaryePkpFF7tjBAqx29L--

Here is the snippet I use and then pass it to gentelman's client.Use(multipart.Data(dt))

fields := map[string]common.Values{"attachment": {"SomeValue"}} dt := common.FormData{ Data: fields, Files: []common.FormFile{{Name: , Reader: r}}, }

It seems that when I pass "Name" in Formfile the fieldname also switches to "Name" . It seems in the file :plugins/multipart/multipart.go (in function writeFile this change happens before passing these values to multipartWriter.CreateFormFile(fileName, file.Name)).

cli.Use(proxy.Set([]string{"http://proxy:3128", "http://proxy2:3128"})) This is from example but in code proxy.Set accept map[string]string with parameters by url schema, and same proxy. It's all nothing - but there is one nuance, map take a schema by request schema. It's not good, but you can adapt. But map can only contain unique keys. So we can use only 1 proxy per 1 scheme. This is not good.

We use gentleman as a client library between services in our product. We began noticing particular services receiving a lot of restart events due to CPU limit violations. When we enabled pprof for those services and took a look at the data, there was a lot of time being spent in addTLS - https://github.com/golang/go/blob/master/src/net/http/transport.go#L1508. This was true even across multiple calls to the same URL and service. This also matched with our observation that the restarts were more frequent with TLS across the full environment instead of it being terminated at the reverse proxy.

We were able to replicate this in a small test outside of the larger application. I've pushed that test to http://github.com/caseyhadden/gentle-http-test. This test runs 10 identical GET calls against httpbin.org using both HTTPS and HTTP. The output below is fairly representative of the type of difference we see. We ran the test cases against our internal server so latency wouldn't have as much of a chance to affect results, but this is what I came up with as something showing the issue and could be shared. The regular "gentleman" usage is consistently around 2x as slow in this microbenchmark. We stuck the grequests library in as another "not bare net/http" representative.

*** HTTPS
2021/02/22 12:49:32 nethttp: 757.704072ms
2021/02/22 12:49:34 gentleman: 1.266130582s
2021/02/22 12:49:35 gentleman transport: 899.396886ms
2021/02/22 12:49:35 grequests: 461.614291ms
*** HTTP
2021/02/22 12:49:36 nethttp: 968.82515ms
2021/02/22 12:49:37 gentleman: 843.205109ms
2021/02/22 12:49:38 gentleman transport: 724.24357ms
2021/02/22 12:49:38 grequests: 769.325955ms

When you check the test, you can see that the difference between "gentleman" and "gentleman transport" instances is that the latter calls cli.Use(transport.Set(http.DefaultTransport)) in order to reuse the HTTP transport instance across the client.

We used a basic load testing tool against one part of the application with such a change. That showed a marked improvement in both throughput and memory usage.

I guess I have a few questions related to this issue:

1. Do you expect to see this difference for TLS and non-TLS connections seemingly related to TLS resumption protocols?
2. Should the default gentleman client have a better setup for this?
3. If not, is there some better way than setting the transport that you're expecting folks to have TLS connections function more performantly?

$ go mod tidy
gopkg.in/h2non/[email protected]: verifying module: checksum mismatch
	downloaded: h1:Qq4Sk2jY7GoYBu8C5rZF/+RU9GdcnzPN9v3z5aBBGg8=
	sum.golang.org: h1:9R3K6CFYd/RdXDLi0pGXwaPnRx/pn5EZlrN3VkNygWc=

SECURITY ERROR
This download does NOT match the one reported by the checksum server.
The bits may have been replaced on the origin server, or an attacker may
have intercepted the download attempt.

For more information, see 'go help module-auth'.

Supposedly, the newer version GO (I'm using GO 1.14.9) would use http/2 with TLS by default on the client-side when the server-side supports http/2. I expect to see HTTP/2 getting logged on the server-side when I use the following code (gentleman.V2 2.0.4)

cli := gentleman.New()
cli.URL("https://mytestserver.com")
cli.Request().Send()

However, I got this logged on the server-side,

[12/Oct/2020:15:43:03 +0000] "GET / HTTP/1.1" 200 14195 "-" "gentleman/2.0.4"

If I do this,

cli := gentleman.New()
cli.Use(transport.Set(http.DefaultTransport))
cli.URL("https://mytestserver.com")
cli.Request().Send()

I got this logged on the server-side,

[12/Oct/2020:15:58:12 +0000] "GET / HTTP/2.0" 200 14175 "-" "gentleman/2.0.4"

Of course, a simple call from the http package like below also got HTTP/2.0 logged on the server-side,

http.Get("https://mytestserver.com")

I've not found the gentleman documentation about this behavior (probably, I'm not thorough enough).

Any comments?