net/http: make it possible to access http.Server via http.ResponseWriter

[bradfitz]

If a handler wants to log an error, the ideal destination would be wherever its Server.ErrorLog points to.

But we can't get to the Server from a handler. Perhaps we can do an optional interface via the ResponseWriter.

Maybe this needs to be tied into various Context efforts.

Related: my comments on https://go-review.googlesource.com/#/c/14161/2/src/net/http/fs.go

/cc @Sajmani @adg @dsymonds @okdave

[pwaller]

Out of interest: is there a nice way to do this in cases where the RepsonseWriter has been interposed by middleware which doesn't implement the relevant methods (for example some gzip middleware)? Or is it simply the case that we must extend every middleware to forward the relevant methods? (Same with Flusher and Hijacker, I guess).

Please could someone cross reference the context efforts? I have missed those and that sounds interesting.

[bradfitz]

The middleware extension pain is why I think we should this into Context (and make the http.Server be a Context value ... http://godoc.org/golang.org/x/net/context#Context see Context.Value).

Then we can even make Flusher be a Context value as well.

@Sajmani, you have links to http context stuff?

[wardn]

It seems dirty to be passing the parent Server into its child Handler. The Context still wouldn't solve the problem of the ServeContent error logger without either a modification to the method signature, one of its parameters, or the addition of a global variable. If you're going to make that kind of modification, wouldn't the following suffice?

package main

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

func main() {
        s := &http.Server{Addr: ":8080"}
        s.ErrorLog = log.New(os.Stdout, "error: ", log.LstdFlags)
        mux := http.NewServeMux()
        mux.HandleFunc("/", handleIt(s.ErrorLog))
        s.Handler = mux
        s.ListenAndServe()
}

func handleIt(logger *log.Logger) func(http.ResponseWriter, *http.Request) {
        return func(w http.ResponseWriter, r *http.Request) {
                logger.Println("fail")
        }
}

I like the idea of the Context, but I'm concerned that it bypasses compile-time safety in favor of flexibility, and it seems like more of a nuclear option to be passing Server down the food chain.

[hallas]

@wardn if you take out the *Server from the context somewhere in one of the handler, it wouldn't require changes to method signature or a global value that is assuming we are discussing a future where access to a context is idiomatic in a standard library http.HandlerFunc.

[Sajmani]

So far there's just client side support in ctxhttp:
https://godoc.org/golang.org/x/net/context/ctxhttp

We've discussed extending this to support server handlers as well, but we
haven't created and concrete CLs yet.
On Sep 2, 2015 11:09 AM, "Brad Fitzpatrick" [email protected]
wrote:

The middleware extension pain is why I think we should this into Context
(and make the http.Server be a Context value ...
http://godoc.org/golang.org/x/net/context#Context see Context.Value).

Then we can even make Flusher be a Context value as well.

@Sajmani https://github.com/Sajmani, you have links to http context
stuff?

—
Reply to this email directly or view it on GitHub
#12438 (comment).

[AmandaCameron]

Maybe a better place to do the context interactions would be to make http.Request satisfy the context.Context interface. This works because it's a concrete type, and so doesn't fall under the Go1 compat promise, for adding stuff. AFAIK nobody needs to recreate / edit those for any reason already.

There's a couple of problems I can think of:

1. kinda breaks the idiom if "context is first argument".
2. IIRC http.Request already has a Done channel on it. so context.Context would need it's Done element renamed. This is possible as the .../context package doesn't fall under the compat promise (yet?)

[Sajmani]

We cannot change the Context interface; it's already very widely used
inside and outside of Google.
We could imagine adding a Context method to http.Request that returns its
context.Context value. This would require moving Context into the standard
library. This hasn't happened yet.
For now, ctxhttp is what we've got.

On Thu, Sep 10, 2015 at 3:59 PM, Amanda Cameron [email protected]
wrote:

Maybe a better place to do the context interactions would be to make
http.Request satisfy the context.Context interface. This works because
it's a concrete type, and so doesn't fall under the Go1 compat promise, for
adding stuff. AFAIK nobody needs to recreate / edit those for any reason
already.

There's a couple of problems I can think of:

1. kinda breaks the idiom if "context is first argument".
2. IIRC http.Request already has a Done channel on it. so context.Context
   would need it's Done element renamed. This is possible as the .../context
   package doesn't fall under the compat promise (yet?)

—
Reply to this email directly or view it on GitHub
#12438 (comment).

[Sajmani]

Edit: a Context method on http.Request could return a value that has the
same methods as context.Context without actually depending on the package.
This would work fine. I'm not sure though whether this is something we
should pursue.

On Mon, Sep 14, 2015 at 9:47 AM, Sameer Ajmani [email protected] wrote:

We cannot change the Context interface; it's already very widely used
inside and outside of Google.
We could imagine adding a Context method to http.Request that returns its
context.Context value. This would require moving Context into the standard
library. This hasn't happened yet.
For now, ctxhttp is what we've got.

On Thu, Sep 10, 2015 at 3:59 PM, Amanda Cameron [email protected]
wrote:

Maybe a better place to do the context interactions would be to make
http.Request satisfy the context.Context interface. This works because
it's a concrete type, and so doesn't fall under the Go1 compat promise, for
adding stuff. AFAIK nobody needs to recreate / edit those for any reason
already.

There's a couple of problems I can think of:

1. kinda breaks the idiom if "context is first argument".
2. IIRC http.Request already has a Done channel on it. so context.Context
   would need it's Done element renamed. This is possible as the .../context
   package doesn't fall under the compat promise (yet?)

—
Reply to this email directly or view it on GitHub
#12438 (comment).

[pwaller]

Being able to associate a context with a *http.Request sounds very nice. However, I don't understand how returning an opaque value from a .Context() method would allow assignment into the *Request. If you did newctx = context.WithValue(req.Context(), myContextualData, value), how do you put newctx back into the *http.Request such that myContextualData is associated with the request from here on out?

[hallas]

@Sajmani

Maybe just expanding ctxhttp with handlers that has both a context, a response writer and a request? That way we as as community can finally rally around that approach.

I don't see how a solution in net/http that keeps the go1 promise could ever be truly satisfactory.

[Sajmani]

Yes, I've discussed ctxhttp handlers with others in another thread. I'll
pull together a change for discussion.
On Sep 14, 2015 11:17 AM, "Christoffer Hallas" [email protected]
wrote:

@Sajmani https://github.com/Sajmani

Maybe just expanding ctxhttp with handlers that has both a context, a
response writer and a request? That way we as as community can finally
rally around that approach.

—
Reply to this email directly or view it on GitHub
#12438 (comment).

[AmandaCameron]

I had an itch in the back of my head that the Done value in http.Request wasn't an actual thing, appears the itch was right, my brain was combining the Cancel attribute with the same type as the Context.Done method. I am not seeing any further conflicts that would prevent a go1-promise-compatable way of making http.Request into a context.Context object, if that's an interested way to go.

[bradfitz]

Moving to Go 1.7, since a good solution to this would almost certainly involve contexts, and contexts in stdlib is being discussed for Go 1.7, not 1.6.

[gopherbot]

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