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

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

Open
nickvellios opened this issue Jan 6, 2017 · 5 comments

Comments

@nickvellios
Copy link

commented Jan 6, 2017

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

This comment has been minimized.

Copy link
Member

commented Jan 6, 2017

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.

@dsnet dsnet added the NeedsFix label Jan 6, 2017

@dsnet dsnet added this to the Unplanned milestone Jan 6, 2017

@dsnet dsnet changed the title Writing Web Applications tutorial - Improper error handling doc/articles/wiki: misleading error handling in Writing Web Applications tutorial Jan 6, 2017

@nickvellios

This comment has been minimized.

Copy link
Author

commented Jan 10, 2017

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

This comment has been minimized.

Copy link
Contributor

commented Jan 11, 2017

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

This comment has been minimized.

Copy link
Author

commented Jan 11, 2017

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

This comment has been minimized.

Copy link
Contributor

commented Jan 11, 2017

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.