Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

the "net/http/httputil.DumpRequest" will destory the context value storage. #25

Closed
elonzh opened this issue Mar 18, 2017 · 6 comments
Closed
Milestone

Comments

@elonzh
Copy link

elonzh commented Mar 18, 2017

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.

@h2non
Copy link
Owner

h2non commented Mar 18, 2017

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

Yeah, that was a design mistake, but I have plans to migrate to net/context in v2.
The good thing is that you're not restricted to use the built-in context provided in gentleman, you can now rely on net/context if you want.

@h2non h2non added this to the v2.0.0 milestone Mar 18, 2017
@elonzh
Copy link
Author

elonzh commented Mar 18, 2017

The most important reason I using this project is the context. (The middleware and plugin system are also great features)

It will be close to perfect for me if this issue be solved.

Thanks for your work. It helped me save a lot of time, although I spent some time to debug. LOL

@h2non
Copy link
Owner

h2non commented Mar 18, 2017

I have just pushed v2 preview release, which relies on stdlib context and gets the rid of all the previous issues with the built-in context.

Code lives in v2 branch:
https://github.com/h2non/gentleman/tree/v2

There's also an outgoing PR that would be eventually merged once the new release is more mature and I had the change to port third-party plugins/packages too: #26

You should be able to use it via:

go get gopkg.in/h2non/gentleman.v2

Let me know how it works.

@h2non
Copy link
Owner

h2non commented Mar 29, 2017

Ping! Did you test this in gentleman@v2? If yes, please let me know how it works!

@elonzh
Copy link
Author

elonzh commented Mar 29, 2017

@h2non Yes! I am using gentleman@v2.

For now, it doesn't cause any issue in my use scene.

@h2non
Copy link
Owner

h2non commented Jul 26, 2017

gentleman@v2 is finally there: https://github.com/h2non/gentleman

@h2non h2non closed this as completed Jul 26, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants