nil buffer fix #136

jaytaylor · 2017-03-31T00:31:33Z

Fixed nil reader content regression introduced with PR #129.

Don't allow buffers with nil contents to be passed to http.NewRequest constructor.

Fixes errors like:

Get https://[SOME-WEBSITE]: stream error: stream ID 1; REFUSED_STREAM

parnurzeal · 2017-04-03T15:47:30Z

Hi @jaytaylor,
Thank you for your PR.
Can you give me some example code? It seems I can't reproduce the error.

jaytaylor · 2017-04-03T20:31:48Z

I agree, it is not immediately obvious what's going on or why this is needed.

From f441e24175ffc865a6354b65375e537201c51382 (Jan 9, 2017), and the next commit, #129 ab5fdf763cb63f22614b6c1a85cba32430c5bec2 "Allow bodies to all HTTP methods" (Feb 10, 2017) there are observable behavioral changes in gorequest which cause breakage on my end.

Example gorequest code

(Extremely basic)

package main

import (
	"github.com/parnurzeal/gorequest"
)

func main() {
	request := gorequest.New()
	request.Get("http://localhost:9000/").EndBytes()
}

Previous working `f441e24` (Jan 9, 2017)

Launch nc -l -v 127.0.0.1 9000 then in another tab run the above go code.

$ nc -l -v 127.0.0.1 9000
GET / HTTP/1.1
Host: localhost:9000
User-Agent: Go-http-client/1.1
Accept-Encoding: gzip
Connection: close

#129 `a578a48` (Feb 10, 2017)

Launch nc -l -v 127.0.0.1 9000 then in another tab run the above go code.

$ nc -l -v 127.0.0.1 9000
GET / HTTP/1.1
Host: localhost:9000
User-Agent: Go-http-client/1.1
Content-Type: application/json
Accept-Encoding: gzip
Connection: close

Notice the new "application/json" content-type header. Since no JSON has been specified, this seems like undesirable behavior.

To be candid, I'm not sure if this PR even resolves that piece.

Also, I agree with the merge request which caused/revealed these issues - imho it's good to allow any request type to have a body attached to it and let people make their own decision.

With that said, looking through net.NewRequest(...) src code, there is a fair amount of logic dependent on whether or not the body is null.

This is why it seems highly undesirable to send a non-nil body buffer wrapping nil contents to http.NewRequest(...). So at the very least, we should stop doing that.

I'll update this PR to address the nil-body aspect for all specially-cased content-types (originally this PR only fixed the JSON case).

jaytaylor · 2017-04-03T20:45:45Z

Update

I've added handling for all cases except for TargetType="multipart". In my opinion this isn't pretty and I'm open to any improved alternative solutions and/or approaches!

jaytaylor · 2017-04-03T20:49:59Z

Additionally, the results of the experiment outlined above with this PR applied are as follows:

$ nc -l -v 127.0.0.1 9000
GET / HTTP/1.1
Host: localhost:9000
User-Agent: Go-http-client/1.1
Accept-Encoding: gzip
Connection: close

Which is back to looking good.

parnurzeal · 2017-04-05T17:50:12Z

Interesting! Thank you for a deep dive into the issue.
I am very impressed.

For improving the code, how about taking req.Header.Set("Content-Type", "xxx") out of those big if-else checks?
We could declare a new variable var contentType string and assign it for each case.
Then, having a check if contentType is not empty, we set Content-Type header.

For example:

var contentType string
if s.TargetType == "json" {
	.
	.
	.
	var contentReader io.Reader
	if contentJson != nil {
		contentReader = bytes.NewReader(contentJson)
		contentType = "application/json"
	}
	req, err = http.NewRequest(s.Method, s.Url, contentReader)
} else if s.TargetType == "form" || s.TargetType == "form-data" || s.TargetType == "urlencoded" {
	.
	.
	.
	var contentReader io.Reader
 	if contentForm != nil {
 		contentReader = bytes.NewReader(contentForm)
 		contentType = "application/x-www-form-urlencoded"
 	}
	req, err = http.NewRequest(s.Method, s.Url, contentReader)
} else if s.TargetType == "..." {
	.
	.
	.
} else if s.TargetType == "multipart" {
	.
	.
	.
	req, err = http.NewRequest(s.Method, s.Url, &buf)
	contentType = mw.FormDataContentType()
} else {
	// let's return an error instead of an nil pointer exception here
	return nil, errors.New("TargetType '" + s.TargetType + "' could not be determined")
}
// Check error from creating a new http request.
if err != nil {
	return nil, err
}
if contentType != "" {
        req.Header.Set("Content-Type", contentType)
}

I know it might not be straightforward to read but we can add more comments to make it clear.
With this, we can reduce some redundant code as well as having less if-else and less headache.

WDYT? I am also open to ideas :)

jaytaylor · 2017-04-10T03:55:49Z

Sure, updating the PR to reflect the proposed approach now :)

jaytaylor · 2017-04-10T05:12:27Z

@parnurzeal Alright, I think we're so close!

Everything seems to work swell, except for the multipart form tests.

A test-case failure and an NPE are being triggered. I'm not entirely clear on what we're testing for in these cases.

Failing cases:

near line 724 in gorequest_test.go:

const _24K = (1 << 20) * 24
err := r.ParseMultipartForm(_24K)
if err != nil {
	t.Errorf("Error: %v", err)
}

gorequest_test.go:727: Error: multipart: NextPart: EOF

near line 756 in gorequest_test.go:

if val, ok := r.MultipartForm.Value["query1"]; ok {
	t.Error("Expected no value", "| but got", val)
}

2017/04/09 22:03:15 http: panic serving 127.0.0.1:49709: runtime error: invalid memory address or nil pointer dereference
goroutine 23 [running]:
net/http.(*conn).serve.func1(0xc420134200)
	/usr/local/go/src/net/http/server.go:1491 +0x12a
panic(0x30b8e0, 0xc4200120e0)
	/usr/local/go/src/runtime/panic.go:458 +0x243
github.com/parnurzeal/gorequest.TestMultipartRequest.func1(0x4bb720, 0xc420128340, 0xc42014a4b0)
	/Users/jtaylor/go/src/github.com/parnurzeal/gorequest/gorequest_test.go:758 +0x928
net/http.HandlerFunc.ServeHTTP(0xc420110480, 0x4bb720, 0xc420128340, 0xc42014a4b0)
	/usr/local/go/src/net/http/server.go:1726 +0x44
net/http.serverHandler.ServeHTTP(0xc4200aa280, 0x4bb720, 0xc420128340, 0xc42014a4b0)
	/usr/local/go/src/net/http/server.go:2202 +0x7d
net/http.(*conn).serve(0xc420134200, 0x4bbda0, 0xc42011c540)
	/usr/local/go/src/net/http/server.go:1579 +0x4b7
created by net/http.(*Server).Serve
	/usr/local/go/src/net/http/server.go:2293 +0x44d

There is also a failure around line 722 which goes away if I force the content-type header to be sent regardless of whether anything has been encoded:

gorequest_test.go:722: Expected Header Content-Type -> multipart/form-data | but got

Your thoughts on how we should proceed are appreciated :)

parnurzeal · 2017-04-11T13:01:56Z

I think the failures in those cases, they are sending zero body content and also checking whether there is a "Content-Type" multi-part or not. And because we changed the behavior not to send "Content-Type", this then makes it fail.

But anyway @jaytaylor , when seeing this error, it made think twice whether we should add logic under it to bounce back to no "Content-Type" if there is no body content.

I am terribly sorry about this but I think we might think too much about this. If a user forces Type("multipart") or Type("form") or ... anything explicitly, we maybe should just let the "Content-Type" be whatever the user wants it to be. The user should be the one who is responsible to not send zero body content her/himself.

So, the easier way to fix this, we need to go look back to the issue cause of when that PR #129 introduced.
That PR removed the switch/case that makes http method like "GET" not sending a pure request without "Content-Type" anymore.
https://github.com/parnurzeal/gorequest/pull/129/files#diff-96bc10f186f846c5018abbf29da04812L1195

And it is just coincidentally I made s.TargetType default to "json" since the beginning of this project. That's why after that PR #129, it sets GET request's content-type to json.
The way to fix this is might be just

set s.TargetType to an empty string when using HTTP GET, instead.

gorequest/gorequest.go

Line 175 in b221adc

func (s *SuperAgent) Get(targetUrl string) *SuperAgent {
don't give an error when s.TargetType is empty but make a request with nil body, instead.
https://github.com/parnurzeal/gorequest/blob/develop/gorequest.go#L1194

Again, really sorry that I might waste your time on the incorrect direction.
But please let me know how my another approach sounds to you.
I might also be wrong on this.

jaytaylor · 2017-04-12T04:01:19Z

This is an interesting turn of perspective!

I agree that if a user forces a type, then it should be respected. At the same time, what we've done here to make a best effort to avoid sending non-nil buffers with nil contents to http.NewRequest(...) still seems worthwhile.

Also, I've found that adding a simple body (Send("foo").) to the exploding test case, now all broken test cases for this PR are passing.

The functionality you're describing doesn't seem mutually exclusive. Is there room for both?

How would you feel about merging this and filing an issue for the target type updates?
(btw I played around with setting TargetType = "" in Get(...), but as you'd expect, that introduces additional test breakage.)

parnurzeal · 2017-04-14T06:39:08Z

Ok cool :) SGTM.
Let's finish up this PR. I have commented a bit on your code.

parnurzeal · 2017-04-11T10:12:46Z

gorequest.go

+		if contentJson != nil {
+			contentReader = bytes.NewReader(contentJson)
+			contentType = "application/json"
+		}
 		req, err = http.NewRequest(s.Method, s.Url, contentReader)
 		if err != nil {


This can be taken out to outside of this if-else.

parnurzeal · 2017-04-11T10:13:11Z

gorequest.go

+		if len(contentForm) != 0 {
+			contentReader = bytes.NewReader(contentForm)
+			contentType = "application/x-www-form-urlencoded"
+		}
 		req, err = http.NewRequest(s.Method, s.Url, contentReader)
 		if err != nil {


parnurzeal · 2017-04-11T10:20:20Z

gorequest.go

+		}
+
+		req, err = http.NewRequest(s.Method, s.Url, contentReader)
+		if err != nil {


Ditto. This can be taken out of if-else code here.

parnurzeal · 2017-04-11T10:21:03Z

gorequest.go

+		req, err = http.NewRequest(s.Method, s.Url, contentReader)
+		if err != nil {
+			return nil, err
+		}
 	} else {
 		// let's return an error instead of an nil pointer exception here
 		return nil, errors.New("TargetType '" + s.TargetType + "' could not be determined")
 	}



We should move those error checks in if-else code to here:
Add:

if err != nil { return nil, err }

- Don't allow buffers with nil contents to be passed to `http.NewRequest' constructor. Fixes errors like: Get https://[SOME-WEBSITE]: stream error: stream ID 1; REFUSED_STREAM See #136 for further information.

jaytaylor · 2017-04-18T20:45:13Z

@parnurzeal Excellent feedback, I had the impulse to do it earlier but refrained on the principle of min-diff.

Let me know how this looks~

Cheers,
Jay

parnurzeal · 2017-04-19T16:06:45Z

Super good!
Thank you for your help resolving this issue :)
It has been very nice and fun to work through this with you.

jaytaylor · 2017-04-19T16:48:56Z

Likewise, thank you! :)

Merge branch 'release-0.2.15'

a578a48

parnurzeal requested changes Apr 14, 2017

View reviewed changes

parnurzeal mentioned this pull request Apr 14, 2017

HTTP Get should not set s.TargetType to "json" but empty string. #138

Open

Fixed nil reader content regression introduced with PR #129.

5da6857

- Don't allow buffers with nil contents to be passed to `http.NewRequest' constructor. Fixes errors like: Get https://[SOME-WEBSITE]: stream error: stream ID 1; REFUSED_STREAM See #136 for further information.

parnurzeal approved these changes Apr 19, 2017

View reviewed changes

parnurzeal merged commit 5ba0c85 into parnurzeal:develop Apr 19, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

nil buffer fix #136

nil buffer fix #136

jaytaylor commented Mar 31, 2017

parnurzeal commented Apr 3, 2017

jaytaylor commented Apr 3, 2017 •

edited

Loading

jaytaylor commented Apr 3, 2017

jaytaylor commented Apr 3, 2017

parnurzeal commented Apr 5, 2017

jaytaylor commented Apr 10, 2017

jaytaylor commented Apr 10, 2017

parnurzeal commented Apr 11, 2017

jaytaylor commented Apr 12, 2017

parnurzeal commented Apr 14, 2017

parnurzeal Apr 11, 2017

parnurzeal Apr 11, 2017

parnurzeal Apr 11, 2017

parnurzeal Apr 11, 2017

jaytaylor commented Apr 18, 2017

parnurzeal commented Apr 19, 2017

jaytaylor commented Apr 19, 2017

nil buffer fix #136

nil buffer fix #136

Conversation

jaytaylor commented Mar 31, 2017

parnurzeal commented Apr 3, 2017

jaytaylor commented Apr 3, 2017 • edited Loading

Example gorequest code

Previous working f441e24 (Jan 9, 2017)

#129 a578a48 (Feb 10, 2017)

jaytaylor commented Apr 3, 2017

jaytaylor commented Apr 3, 2017

parnurzeal commented Apr 5, 2017

jaytaylor commented Apr 10, 2017

jaytaylor commented Apr 10, 2017

parnurzeal commented Apr 11, 2017

jaytaylor commented Apr 12, 2017

parnurzeal commented Apr 14, 2017

parnurzeal Apr 11, 2017

Choose a reason for hiding this comment

parnurzeal Apr 11, 2017

Choose a reason for hiding this comment

parnurzeal Apr 11, 2017

Choose a reason for hiding this comment

parnurzeal Apr 11, 2017

Choose a reason for hiding this comment

jaytaylor commented Apr 18, 2017

parnurzeal commented Apr 19, 2017

jaytaylor commented Apr 19, 2017

jaytaylor commented Apr 3, 2017 •

edited

Loading

Previous working `f441e24` (Jan 9, 2017)

#129 `a578a48` (Feb 10, 2017)