Golang *bytes.Buffer nil causes fatal error

Question

I had the same issue as https://github.com/golang/go/issues/26666 because I have a wrap function for my http requests.

Sometimes I need to request:

body := new(bytes.Buffer)
json.NewEncoder(body).Encode(h)
req("POST", "http://example.com", body)

And sometimes it's simply:

req("GET", "http://example.com", nil)
runtime error: invalid memory address or nil pointer dereference

I ended up with:

req("GET", "http://example.com", new(bytes.Buffer))

But I'm not sure if it's the right thing to do.

The function:

func req(method string, url string, body *bytes.Buffer) int {
req, err := http.NewRequest(method, url, body)
req.Header.Set("Content-Type", "application/json")
req.SetBasicAuth(user, psw)
resp, err := client.Do(req)
checkErr(err)
if resp.StatusCode > 500 {
    time.Sleep(30 * time.Second)
    resp, err = client.Do(req)
    checkErr(err)
}
defer resp.Body.Close()
return resp.StatusCode
}

Updated function:

func req(method string, url string, body io.Reader) int {
    req, err := http.NewRequest(method, url, body)
    req.Header.Set("Content-Type", "application/json")
    req.SetBasicAuth(user, psw)
    resp, err := client.Do(req)
    checkErr(err)
    defer resp.Body.Close()
    if resp.StatusCode >= 500 {
        time.Sleep(30 * time.Second)
        req, err := http.NewRequest(method, url, body)
        req.Header.Set("Content-Type", "application/json")
        req.SetBasicAuth(user, psw)
        resp, err := client.Do(req)
        checkErr(err)
        defer resp.Body.Close()
    }
    return resp.StatusCode
}

func checkErr(err error) {
    if err != nil {
        log.Fatal(err)
    }
}

Hey, it works! :) I was concerned about whether this looks stupid. Learning go on the go :) — raimondsL, Feb 01 '19 at 08:10
Don't ignore the error returned by http.NewRequest. What does checkErr do? If it doesn't terminate the program you have to return after calling it. And you are not closing the bodies of 501+ responses (did you mean `>=`?) — Peter, Feb 01 '19 at 08:13
Your code has one subtle issue, if the first `resp` has StatusCode>500 you make another request and use the result overwrite the original `resp` value, without ever calling `Body.Close()` on the original `resp`. This, given enough time and enough 500+ responses, will result in your program running out of file descriptors and eventually crashing. You need to close both responses' bodies. — mkopriva, Feb 01 '19 at 08:17
One more thing: you cannot reuse the req value for retries, because its body has already been read. You have to create a new request. — Peter, Feb 01 '19 at 08:21
I wonder if the correct approach is defer resp.Body.Close() or resp.Body.Close() w/o defer — raimondsL, Feb 01 '19 at 09:44

icza · Accepted Answer · 2019-02-01T08:21:31.670

The body in http.NewRequest() is optional, so passing nil is acceptable when you're doing GET requests.

The problem is that the body parameter of http.NewRequest is an interface type: io.Reader, and you're attempting to pass a value of a concrete type *bytes.Buffer. What happens is that this nil pointer will be wrapped in a non-nil interface value, and that will be passed to http.NewRequest as the body.

If you don't have a body, pass nil explicitly, like this:

func req(method string, url string, body *bytes.Buffer) int {
    var bodyToPass io.Reader
    if body != nil {
        bodyToPass = body
    }
    req, err := http.NewRequest(method, url, bodyToPass)

    // ....
}

And then you can call it like:

req("GET", "http://example.com", nil)

Although best would be if your req() function would take io.Reader in the first place, so you don't have to check its value explicitly:

func req(method string, url string, body io.Reader) int {
    req, err := http.NewRequest(method, url, body) // You may pass it as-is

    // ....
}

And you can call it with nil or with a non-nil *bytes.Buffer too:

req("GET", "http://example.com", nil) // OK

req("POST", "http://example.com", bytes.NewBufferString("data")) // Also OK

For more details, see Hiding nil values, understanding why golang fails here

Thank you for help. I've updated "func req". Do you think it's right? — raimondsL, Feb 01 '19 at 09:07
@RaimondsLinde Calling `log.Fatal()` is hardly ever right. You might do that in a quick demo, but in production code you'd never want to do that. Instead return the error and let the caller deal with it. — icza, Feb 01 '19 at 10:20

Golang *bytes.Buffer nil causes fatal error

1 Answers1