Thursday, February 22, 2024

Don't close resp.Body on error

So my server finally crashed for the first time today luckily I was running it through systemd which captured the stderr (yay!) so I saw what the problem was. Turns out it panicked in the function where I ping the health check URL. 

So, for context, my web service has a goroutine which continually pings a health check URL every few seconds.

And today, that http Get request failed, returning an error. 

So here's the code that failed:

        resp, err := http.Get(url)
        if err != nil {
            log.Println(err)
        }
        resp.Body.Close()

See if you can notice the bug here. Yes, the problem is that when err is not nil, the resp can be a nil pointer. So doing resp.Body.Close() will cause a panic. Which is what happened.

The correct thing is to return when err != nil. Do not call resp.Body.Close() when err is non-nil.


I asked GPT4 this question and it actually spotted the bug immediately:

This code appears incomplete, and there is a critical bug that could cause a program execution to panic. The issue is that http.Get(url) could fail and return a nil response. If that occurs, calling resp.Body.Close() will trigger a runtime panic because you're trying to access a property on a nil value.

You should always check whether err is nil before working with the resp object to avoid such issues. Here's a corrected version:

        resp, err := http.Get(url)
        if err != nil {
            log.Println(err)
            return
        }
        defer resp.Body.Close()

By returning from the function after logging the error, you're ensuring resp.Body.Close() isn't called when resp is nil.

Also, it's good practice to use the defer statement to close resources like this, which ensures resp.Body.Close() is called before the function returns, in any case. It helps prevent leaks of resources.


Maybe I should ask GPT4 to check over all of my code...

No comments:

Post a Comment