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

net/http: http.ServeFile and http.ServeContent Explicitly Set http.StatusOK #11269

Closed
elithrar opened this issue Jun 18, 2015 · 5 comments

Comments

Projects
None yet
4 participants
@elithrar
Copy link

commented Jun 18, 2015

What version of Go are you using (go version)?
go version go1.4.2 darwin/amd64

What operating system and processor architecture are you using?
OS X 10.10.3 x86-64

What did you do?
Set w.WriteHeader(http.StatusInternalServerError) prior to calling http.ServeFile

What did you expect to see?
A HTTP 500 status.

What did you see instead?
A HTTP 200 status (and the usual multiple header writes error for messing it up).

Currently http.ServeFile (which calls http.ServeContent and then eventually http.serveContent) implicitly sets the status code to HTTP 200 as per this line in src/http/fs.go

From what I can see, removing this line would result in net/http implicitly calling w.WriteHeader(StatusOK) as per usual (http://golang.org/src/net/http/server.go#L990) - which is the behaviour I'd expected until running into this.

Proposed Fix

  • Modify http.serveContent to only call w.WriteHeader(code) on an error condition. Set var code int (i.e. zero value) instead of defaulting to code := StatusOK.
  • If code still has the zero value before we write to the ResponseWriter, then don't call WriteHeader.
  • This allows the server to either: a) write the header set by the package user or; b) write the implicit http.StatusOK if no header has been set by the time we're ready to write back.

Writing StatusOK in serveContent appears to be redundant.

@elithrar elithrar changed the title http.ServeFile and http.ServeContent Explicitly Set http.StatusOK net/http: http.ServeFile and http.ServeContent Explicitly Set http.StatusOK Jun 18, 2015

@ianlancetaylor ianlancetaylor added this to the Go1.6 milestone Jun 18, 2015

@bradfitz

This comment has been minimized.

Copy link
Member

commented Jun 18, 2015

Only the first WriteHeader wins. How did you get a 200?

Can you post a minimal example demonstrating the issue?

@elithrar

This comment has been minimized.

Copy link
Author

commented Jun 18, 2015

In this case the WriteHeader is being called explicitly in serveContent prior to the check for if !w.WroteHeader at the server level. A package user is always going to be "doubling up" with serveContent in this case.

The use-case (for context) is serving static files to respond to HTTP 50x's or HTTP 404's when using Go as a web server without a reverse proxy like nginx in front.

package main

import (
    "log"
    "net/http"
    "os"
    "path/filepath"
)

func main() {
    // e.g. /Users/you/example.com/
    static := os.Getenv("STATIC_DIR")

    // Set this to 200, 500, 404, etc - same result - multiple WriteHeader
    // calls.
    http.Handle("/", serveErrorPage(500, static))
    http.ListenAndServe(":8000", nil)
}

func serveErrorPage(code int, dir string) http.Handler {
    fn := func(w http.ResponseWriter, r *http.Request) {
        // Call duplicated by src/net/http/fs.go#L254
        w.WriteHeader(code)
        path := filepath.Join(dir, r.URL.Path)
        log.Println(path)
        http.ServeFile(w, r, path)
    }

    return http.HandlerFunc(fn)
}

A fairly contrived/minimal example but should demonstrate the issue. Set STATIC_DIR=/some/dir/ to a directory containing something simple (a HTML or text file) and send it a request.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Jun 24, 2015

I tried your code. The server is returning a 500 header. Maybe you're confused that the HTML body doesn't say 500?

$ curl -v http://localhost:8000/
* Adding handle: conn: 0x7fc74480aa00
* Adding handle: send: 0
* Adding handle: recv: 0
* Curl_addHandleToPipeline: length: 1
* - Conn 0 (0x7fc74480aa00) send_pipe: 1, recv_pipe: 0
* About to connect() to localhost port 8000 (#0)
*   Trying ::1...
*   Trying 127.0.0.1...
* Connected to localhost (127.0.0.1) port 8000 (#0)
> GET / HTTP/1.1
> User-Agent: curl/7.30.0
> Host: localhost:8000
> Accept: */*
> 
< HTTP/1.1 500 Internal Server Error
< Date: Wed, 24 Jun 2015 10:17:19 GMT
< Content-Type: text/plain; charset=utf-8
< Transfer-Encoding: chunked
< 
<pre>
<a href="all.bash">all.bash</a>
<a href="all.bat">all.bat</a>
<a href="all.rc">all.rc</a>
...

The problem with trying to use ServeContent after doing WriteHeader(500) is that the header will already be flushed at that point, and ServeContent will be unable to set the Content-Type, etc. So you'll need to do that part anyway.

But if you do that, calling ServeContent after your own WriteHeader(500) seems to work fine.

Feel free to re-open this bug if I'm missing something.

@bradfitz bradfitz closed this Jun 24, 2015

@elithrar

This comment has been minimized.

Copy link
Author

commented Jun 24, 2015

Thanks for the reply @bradfitz. Indicating I was getting a HTTP 200 was a mistake—that wasn't the case (must have been conflating it with some other testing I was doing at the time).

I still see the following from the server's log when calling w.WriteHeader(500) prior to ServeFile:

2015/06/25 07:28:21 static/page.html
2015/06/25 07:28:21 http: multiple response.WriteHeader calls

... and the corresponding curl command:

~/Desktop curl -v http://127.0.0.1:8000/style.css
* Hostname was NOT found in DNS cache
*   Trying 127.0.0.1...
* Connected to 127.0.0.1 (127.0.0.1) port 8000 (#0)
> GET /style.css HTTP/1.1
> User-Agent: curl/7.37.1
> Host: 127.0.0.1:8000
> Accept: */*
>
< HTTP/1.1 500 Internal Server Error
< Date: Wed, 24 Jun 2015 23:31:14 GMT
< Content-Type: text/plain; charset=utf-8
< Transfer-Encoding: chunked
<
html, body {
    background-color: #eee;
    font-family: Helvetica, Arial, sans-serif;
}
* Connection #0 to host 127.0.0.1 left intact

This is with the program unmodified from above.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Jun 25, 2015

I see. I don't think this is worth changing. Once you've already sent the headers, ServeContent can no longer do If-Modified-Since or Range requests, etc. At that point there's little functionality it's providing that you can't more easily do yourself for your custom 500 handler.

I think changing this behavior would just be encouraging people in the wrong direction. I'm content saying that it should only be used prior to writing headers.

@golang golang locked and limited conversation to collaborators Jun 25, 2016

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.