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

Go 1.7 context #41

Closed
wants to merge 1 commit into from
Closed

Go 1.7 context #41

wants to merge 1 commit into from

Conversation

ejholmes
Copy link
Contributor

Related #40

This is a WIP branch to add Go 1.7 support and use package context in the stdlib. I don't really expect this to actually get merged anytime soon, but opening a PR so we can comment on it.

The biggest change is removing the httpx.Handler interface, and changing everything to implement http.Handler. The way I see it, we have 2 choices:

  1. Change the signature of httpx.Handler to:

    - ServeHTTPContext(ctx context.Context, w http.ResponseWriter, r *http.Request) error
    + ServeHTTPContext(w http.ResponseWriter, r *http.Request) error
  2. Or just remove it.

I'm in favor of just removing it and encouraging usage of http.Handler, since it's more portable across libraries in the Go ecosystem. It's fairly easy for consumers to add their own type that returns an error (in fact, that's what Empire's http server does in Go 1.7). Doing this also means we no longer need to wrap gorilla/mux.

There's also some backwards incompatible behavior changes:

  • middleware.NewRelicTracer, and middelware.NewResponseTimeReporter requires that the middleware be added after route dispatch has happened by gorilla mux. This is because, we were originally relying on some hacks in httpx.Router to support pulling out the route template. But it's quite a bit easier/cleaner if we just rely on mux.CurrentRoute. This change also opens it up to support other routes more easily. Again, I think this is easy to deal with by encouraging a design like this.

/cc @v-yarotsky @sanjayprabhu

@v-yarotsky
Copy link
Contributor

v-yarotsky commented Jun 13, 2016

I have a question that is not directly related to this.
Say, we have a main router that mounts and unauthed endpoints (like /health) and authed endpoints (another router).
How hard would it be to obtain the full route template for nested routers?

UPD:
Approach 2 is compelling, however I like the idea of having our own router implementation (even if it just wraps a 3rd party package) just because it allows us to standardize our stack (including middleware).

func templatePath(r *http.Request) string {
path := "unknown"

if route := mux.CurrentRoute(r); route != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it possible to shove current route template into request context upon dispatching?

@ejholmes
Copy link
Contributor Author

Do you mean, something like this?

package main

import (
    "fmt"
    "net/http"
    "net/http/httptest"

    "github.com/gorilla/mux"
)

func handler(w http.ResponseWriter, r *http.Request) {
    route := mux.CurrentRoute(r)
    fmt.Println(route.GetPathTemplate())
}

func main() {
    r1 := mux.NewRouter()

    r2 := r1.PathPrefix("/path").Subrouter()
    r2.HandleFunc("/nested", handler)

    r, _ := http.NewRequest("GET", "/path/nested", nil)
    w := httptest.NewRecorder()

    r1.ServeHTTP(w, r)
}

In that case, route.GetPathTemplate() is going to return /path/nested.

@v-yarotsky
Copy link
Contributor

@ejholmes nice!

@@ -1,7 +1,7 @@
language: go

go:
- 1.4
- tip
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why tip?

@bmarini bmarini mentioned this pull request Feb 15, 2018
@ejholmes ejholmes closed this Jun 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants