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: "Writing Web Applications" example writes the reponse header twice #27789

Open
Houndie opened this issue Sep 21, 2018 · 5 comments · May be fixed by #27809

Comments

@Houndie
Copy link

commented Sep 21, 2018

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

go1.11

Does this issue reproduce with the latest release?

Yes

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


GOARCH="amd64"
GOBIN="/home/jabenze/go/bin"
GOCACHE="/home/jabenze/.cache/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/jabenze/go"
GOPROXY=""
GORACE=""
GOROOT="/usr/lib/go"
GOTMPDIR=""
GOTOOLDIR="/usr/lib/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build876174949=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Follow the "Writing Web Applications" tutorial here, up through the error handling section here: https://golang.org/doc/articles/wiki/#tmp_9. L

Look at the function renderTemplate, reproduced here:

func renderTemplate(w http.ResponseWriter, tmpl string, p *Page) {
    t, err := template.ParseFiles(tmpl + ".html")
    if err != nil {
        http.Error(w, err.Error(), http.StatusInternalServerError)
        return
    }
    err = t.Execute(w, p)
    if err != nil {
        http.Error(w, err.Error(), http.StatusInternalServerError)
    }
}

Modify the Page data structure by removing a field (or modify the template to reference a nonexisting field) so that the line: err = t.Execute(w, p) will cause an error to be created. Observe the response code sent by the application. The following line: http.Error(w, err.Error(), http.StatusInternalServerError) would imply the response code to be 500, however, the application returns 200.

This is because t.Execute is calling w.Write which, because w.WriteHeader has not yet been called, causes an implicit call to w.WriteHeader(200). Further calls to w.WriteHeader, such as the one inside http.Error are ignored.

There is no way to able to send a 500 error code in the case of all errors, since w.Write itself can return an error, and at some point, one has to simply log an error and return without setting an error code. Optionally, the example could render to a bytes.Buffer so that template rendering errors could be caught.

What did you expect to see?

Based on the code, a 500 error code

What did you see instead?

Error code 200.

@dmitshur

This comment has been minimized.

Copy link
Member

commented Sep 21, 2018

This is a valid issue, good catch! Thank you for the well-written report and good analysis.

We should discuss how to resolve it. As you said, one option is to render the template to a bytes.Buffer first, and only write to the http.ResponseWriter once all the operations that might fail have succeeded, leaving only copying the buf into rw. However, this might not fit well with the rest of the tutorial.

There are other ways to fix this too, and it's important to come up with a solution that makes the most sense in the context of the "Writing Web Applications" tutorial and what the reader is expected to know or be exposed to by then.

/cc @adg as the original author of the article, I believe.

@dmitshur dmitshur changed the title wiki: "Writing Web Applications" example writes the reponse header twice doc/articles/wiki: "Writing Web Applications" example writes the reponse header twice Sep 21, 2018

@Houndie

This comment has been minimized.

Copy link
Author

commented Sep 21, 2018

The only problem with the bytes buffer rendering is that, if the tutorial is going to cover proper error handling, we still run into the same problem down the line.

buf := &bytes.Buffer{}
if err := t.Execute(buf, p); err != nil {
   http.Error(w, err.Error(), http.StatusInternalServerError)
   return
}

if err := w.Write(buf.Bytes()); err != nil {
   // can't call http.Error here for the same reason as before.  We've caught 
   // template problems, but we still can't do exhaustive error handling with http.Error
}

My suggestion for the tutorial would probably just be to keep writing directly to the ResponseWriter and log the template rendering error, and then have either a comment or short blurb in the tutorial about why we can't write the header twice. It's correct code and doesn't complicate the tutorial too much.

Dunno if there's a simpler solution that I'm missing though.

@adg

This comment has been minimized.

Copy link
Contributor

commented Sep 21, 2018

My suggestion for the tutorial would probably just be to keep writing directly to the ResponseWriter and log the template rendering error

This is what I came here to say. Template rendering errors are for the application author anyway, not the user, and should typically only happen during development of the program. Logging it here would be ideal, and a couple of sentences explaining this would actually improve the tutorial.

Houndie added a commit to Houndie/go that referenced this issue Sep 22, 2018
doc/articles/wiki: Don't write response header twice in tutorial.
In the "Writing Web Applications" example, we attempt to write the
response header twice when handling errors from `template.Execute`.
Since this doesn't work in practice, it shouldn't be done in the
tutorial. Instead, simply log the error, and note the reason.

Fixes golang#27789
Houndie added a commit to Houndie/go that referenced this issue Sep 22, 2018
doc/articles/wiki: not write response header twice in tutorial
In the "Writing Web Applications" example, we attempt to write the
response header twice when handling errors from template.Execute.
Since this doesn't work in practice, it shouldn't be done in the
tutorial. Instead, simply log the error, and note the reason.

Fixes golang#27789
Houndie added a commit to Houndie/go that referenced this issue Sep 22, 2018
doc/articles/wiki: remove misleading code in tutorial
In the "Writing Web Applications" example, we attempt to write the
response header twice when handling errors from template.Execute.
Since this doesn't work in practice, it shouldn't be done in the
tutorial. Instead, simply log the error, and note the reason.

Fixes golang#27789
@Houndie Houndie referenced a pull request that will close this issue Sep 22, 2018
@gopherbot

This comment has been minimized.

Copy link

commented Sep 22, 2018

Change https://golang.org/cl/136757 mentions this issue: doc/articles/wiki: remove misleading code in tutorial

@Houndie

This comment has been minimized.

Copy link
Author

commented Sep 22, 2018

I'm aware this still has the "Needs Decision" tag associated with it, but since all were in agreement so far, I went ahead and wrote something up to fix it. I'm not the best writer, but it's a decent start at least.

Houndie added a commit to Houndie/go that referenced this issue Sep 24, 2018
doc/articles/wiki: remove misleading code in tutorial
In the "Writing Web Applications" example, we attempt to write the
response header twice when handling errors from template.Execute.
Since this doesn't work in practice, it shouldn't be done in the
tutorial. Instead, simply log the error, and note the reason.

Fixes golang#27789
Houndie added a commit to Houndie/go that referenced this issue Sep 24, 2018
doc/articles/wiki: remove misleading code in tutorial
In the "Writing Web Applications" example, we attempt to write the
response header twice when handling errors from template.Execute.
Since this doesn't work in practice, it shouldn't be done in the
tutorial. Instead, simply log the error, and note the reason.

Fixes golang#27789

@dmitshur dmitshur added NeedsFix and removed NeedsDecision labels Sep 24, 2018

Houndie added a commit to Houndie/go that referenced this issue Oct 18, 2018
doc/articles/wiki: remove misleading code in tutorial
In the "Writing Web Applications" example, we attempt to write the
response header twice when handling errors from template.Execute.
Since this doesn't work in practice, it shouldn't be done in the
tutorial. Instead, simply log the error, and note the reason.

Fixes golang#27789
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.