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

cmd/vet: check for http.Error followed by other statements in handler? #15205

Open
dsnet opened this issue Apr 8, 2016 · 7 comments
Open

cmd/vet: check for http.Error followed by other statements in handler? #15205

dsnet opened this issue Apr 8, 2016 · 7 comments
Assignees
Milestone

Comments

@dsnet
Copy link
Member

@dsnet dsnet commented Apr 8, 2016

Using go1.6

I recently saw code that did the following:

func serveHTTP(resp http.ResponseWriter, req *http.Request) {
    ...

    if err := foo(); err != nil {
        http.Error(resp, err.Error(), http.StatusInternalServerError)
    }

    if err := bar(); err != nil {
        http.Error(resp, err.Error(), http.StatusInternalServerError)
    }
}

The assumption made was that http.Error() terminates the current handler in some magical way. Instead, Error simply sets the headers and writes the body message, and it is the programmer's responsibility to return. We should document this.

@bradfitz bradfitz added this to the Go1.7 milestone Apr 9, 2016
@bradfitz bradfitz self-assigned this Apr 9, 2016
@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Apr 9, 2016

I also wonder whether we should write a go vet check for this too. But we should only do so if this is a common problem. Is it easy for somebody to AST-grep all public Go source code and look for an http.Error with reachable statements afterwards? (/cc @sqs, @alandonovan)

We can start with documenting this first, of course.

@gopherbot
Copy link

@gopherbot gopherbot commented Apr 11, 2016

CL https://golang.org/cl/21836 mentions this issue.

@alandonovan
Copy link
Contributor

@alandonovan alandonovan commented Apr 11, 2016

Based on a quick scan of Google's Go code base, I think such a check would find several errors, but not without false positives. Sometimes an http.Error call and its subsequent return statement are separated by logging statements or error-counter increments. We could exempt any function calls with "log" or "Error" (or within Google, "Add") in their names. The false positives are easy to work around by transposing statements.

@gopherbot gopherbot closed this in 00681ee Apr 11, 2016
@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Apr 11, 2016

Re-opening to consider vet checks.

@bradfitz bradfitz reopened this Apr 11, 2016
@bradfitz bradfitz modified the milestones: Unplanned, Go1.7 Apr 11, 2016
@bradfitz bradfitz removed the Documentation label Apr 11, 2016
@bradfitz bradfitz removed their assignment Apr 11, 2016
@bradfitz bradfitz changed the title net/http: document that Error() does not terminate the current handler cmd/vet: check for http.Error followed by other statements in handler? Apr 11, 2016
@dmitshur
Copy link
Member

@dmitshur dmitshur commented Apr 12, 2016

Is it easy for somebody to AST-grep all public Go source code and look for an http.Error with reachable statements afterwards?

/cc @dominikh ;)

@gopherbot
Copy link

@gopherbot gopherbot commented Jan 15, 2020

Change https://golang.org/cl/214859 mentions this issue: go/analysis: add analyzer for http.Error missing termination

@odeke-em
Copy link
Member

@odeke-em odeke-em commented Jan 22, 2020

Thanks for filing and following up on this bug!
A couple of years back (in 2016), on a Saturday afternoon, while @bradfitz and I were hacking on Go bugs, he mentioned this bug to me and I just remembered it about 3 weeks ago and so I've mailed out CL 214859 which implements the static analysis for pretty much all the cases except the benign branches that should be pruned i.e.

func h(w http.ResponseWriter, r *http.Request) {
    if true {
       http.Error(w, msg, code)
    }
}
func h(w http.ResponseWriter, r *http.Request) {
    {
        http.Error(w, msg, code)
    }
}

For all the other cases with missing terminating statements, and heuristically known terminating statements e.g. (log.Fatal*) it'll report the missing termination statements.

I hope to get this in for Go1.15, but couldn't do it ASAP due an emergency.

@odeke-em odeke-em self-assigned this Jan 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants
You can’t perform that action at this time.