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: make it possible to access http.Server via http.ResponseWriter #12438

Closed
bradfitz opened this issue Sep 1, 2015 · 14 comments
Closed

Comments

@bradfitz
Copy link
Contributor

bradfitz commented Sep 1, 2015

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

@bradfitz bradfitz self-assigned this Sep 1, 2015
@bradfitz bradfitz added this to the Go1.6 milestone Sep 1, 2015
@pwaller
Copy link
Contributor

pwaller commented Sep 2, 2015

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
Copy link
Contributor Author

bradfitz commented Sep 2, 2015

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
Copy link

wardn commented Sep 3, 2015

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
Copy link

hallas commented Sep 3, 2015

@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
Copy link
Contributor

Sajmani commented Sep 4, 2015

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" notifications@github.com
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
Copy link

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
Copy link
Contributor

Sajmani commented Sep 14, 2015

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 notifications@github.com
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
Copy link
Contributor

Sajmani commented Sep 14, 2015

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 sameer@golang.org 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 notifications@github.com
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
Copy link
Contributor

pwaller commented Sep 14, 2015

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
Copy link

hallas commented Sep 14, 2015

@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
Copy link
Contributor

Sajmani commented Sep 24, 2015

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" notifications@github.com
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
Copy link

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 bradfitz modified the milestones: Go1.7Early, Go1.6 Dec 3, 2015
@bradfitz
Copy link
Contributor Author

bradfitz commented Dec 3, 2015

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
Copy link

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

@golang golang locked and limited conversation to collaborators Apr 12, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

7 participants