Skip to content
This repository has been archived by the owner on Jun 6, 2023. It is now read-only.

ioutil.ReadAll considered harmful #41

Closed
froodian opened this issue Apr 25, 2016 · 6 comments · Fixed by #43
Closed

ioutil.ReadAll considered harmful #41

froodian opened this issue Apr 25, 2016 · 6 comments · Fixed by #43

Comments

@froodian
Copy link

http://jmoiron.net/blog/crossing-streams-a-love-letter-to-ioreader/ has some context on reasons to avoid a ReadAll. This fact is reflected in the current source - from the source:

    // TODO: could decode while reading instead
    body, err := ioutil.ReadAll(resp.Body)
    if err != nil {
        return "", err
    }

    var response response
    json.Unmarshal(body, &response)

If I'm undersatnding correctly, I believe this TODO could be done by essentially doing

var response response
err = json.NewDecoder(resp.Body).Decode(&response)
if err != nil {
    return "", err
}

and it would be beneficial for performance

@nathany
Copy link
Contributor

nathany commented Apr 26, 2016

Thanks for the article. I'll take a look before making the switch.

I've seen some reasons to read the entire body for HTTP/1.1 but I don't think they apply for HTTP/2.0. Decode and ReadAll behave a bit differently, but I imagine Decode would work just as well in this case.

Nathan.

@nathany
Copy link
Contributor

nathany commented Apr 26, 2016

Pretty convincing article. :D

@froodian
Copy link
Author

yeah I found it compelling, glad you did too, it certainly caused me to change some of my code, haha

@nathany
Copy link
Contributor

nathany commented Apr 26, 2016

see #43. tests are passing :-)

@froodian
Copy link
Author

Awesome! :D

@nathany
Copy link
Contributor

nathany commented Apr 26, 2016

I like that it removes a dependency to, no need to bring in ioutil.

Cheers.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants