search

golang / go

The Go programming language
https://go.dev
BSD 3-Clause "New" or "Revised" License
123.87k stars 17.65k forks source link

doc/articles/wiki: misleading error handling in Writing Web Applications tutorial #18543

Open nickvellios opened 7 years ago

nickvellios commented 7 years ago

Please answer these questions before submitting your issue. Thanks!

What version of Go are you using (go version)?

1.7

What operating system and processor architecture are you using (go env)?

GOARCH="amd64" GOBIN="" GOEXE="" GOHOSTARCH="amd64" GOHOSTOS="darwin" GOOS="darwin" GOPATH="/Users/nick/golang" GORACE="" GOROOT="/usr/local/go" GOTOOLDIR="/usr/local/go/pkg/tool/darwin_amd64" CC="clang" GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/c9/n428syrn3xl63_wf458yzw9w0000gq /T/go-build862609477=/tmp/go-build -gno-record-gcc-switches -fno-common" CXX="clang++" CGO_ENABLED="1"

What did you do?

On line 66 of this example: https://golang.org/doc/articles/wiki/final.go If ExecuteTemplate begins to write to the http.ResponseWriter before an error is thrown, http.Error will not properly return an error to the user.

New Gophers will see this (as I did) and assume ExecuteTemplate will never write to the buffer if an error is thrown. This is not correct.

Example case: https://play.golang.org/p/E_-jtdCLjk

What did you expect to see?

Write to a temporary buffer first, if no error is caught then write that to the http.ResponseWriter.

What did you see instead?

http.Error(w, err.Error(), http.StatusInternalServerError)

dsnet commented 7 years ago

Seems like a reasonable change to the example. I've been bitten by this mistake before as well.

A part of me wonders how expensive it would be for template to do up-front error-checking to ensure that it never starts writing unless it knows it will encounter no template issues.

nickvellios commented 7 years ago

dsnet, good question. Without an intimate understanding of the underlying template package I have some doubts. If templates are parsed ahead of time and it can be ensured that variable nil pointer exceptions and the like don't throw a panic, it may be doable with minimal overhead.

kisielk commented 7 years ago

It's probably a bad idea to encourage printing the error from the template engine to the client in the first place. That seems like something that should be logged and a generic error message sent with the 500 response. Maybe that's getting too deep for the example though :)

nickvellios commented 7 years ago

Agreed. However, I wonder where that line should be drawn between a production level example and easy to follow teaching material. Maybe a part 1 with the basics and part 2 showing how to secure it is overdue. From reading many of the requests on /r/golang I can see web app curiosity is strong.

kisielk commented 7 years ago

Yeah, it's probably out of scope of this issue.

As it stands, the whole "Error handling" section of that document is incorrect because it's supposed to demonstrate returning an error to the client if there is a problem with the template execution, but it will in fact return http.StatusOK since the write has already begun. That can easily be fixed by the use of a buffer.