Chi v2 + context in http.Request in go1.7

[VojtechVitek]

Opening PR from @pkieltyka's WIP v2 branch, so we can discuss final chi v2 features and push this forward :)

btw: Big kudos to @pkieltyka for opening discussion about having "context" in golang stdlib http pkg in the first place:

• https://go-review.googlesource.com/#/c/15620/
• https://groups.google.com/forum/#!msg/golang-codereviews/zpZ3TQAwcX8/KGWXf6r0CgAJ

[VojtechVitek]

pkieltyka commented 6 days ago

btw, what do you guys think about 3c613e2 ? for chi v2, I believe the names Group() and Stack() reflect better the intention of the methods. That is, to group routes by a routing pattern and to stack one router ontop of another without a pattern.

[VojtechVitek]

To be honest, I like the original r.Route("/route", subrouter) and r.Group(subrouter) better. Stack() sounds confusing to me, since it doesn't really trigger "custom middleware stack" in my head. And I'm not huge fan of changing .Group() logic either.

[VojtechVitek]

btw: How about removing .Route(), changing .Mount()'s signature (so it's not like .Handle()) and keep .Group() only?

type Router interface {
    http.Handler

    Group(fn func(r Router)) Router
    Mount(pattern string, r Router)
    Handle(pattern string, h http.Handler)
    //...
}

// --------

r := chi.NewRouter()
r.Use(ForAllCtx)

r.Group(func(r chi.Router) {
    r.Use(SpecialCtx)
    r.Get("/", handler)
})

r.Mount("/ping", subrouter)

r.Mount("/:articleID", r.Group(func(r chi.Router) {
    r.Use(ArticleCtx)
    r.Get("/", getArticle)
}))

[pkieltyka]

I think you're right and I'll go back to original Route() and Group() for now

Route is like a Mount that accepts a function instead of the router, I like it for building the tree as shorthand

On Jun 15, 2016, at 11:32 PM, Vojtech Vitek notifications@github.com wrote:

In chi.go:

• Connect(pattern string, handlers ...interface{})
• Head(pattern string, handlers ...interface{})
• Get(pattern string, handlers ...interface{})
• Post(pattern string, handlers ...interface{})
• Put(pattern string, handlers ...interface{})
• Patch(pattern string, handlers ...interface{})
• Delete(pattern string, handlers ...interface{})
• Trace(pattern string, handlers ...interface{})
• Options(pattern string, handlers ...interface{})
• Use(middlewares ...func(http.Handler) http.Handler)
• Stack(fn func(r Router)) Router
• Group(pattern string, fn func(r Router)) Router
  btw: How about removing .Route(), changing .Mount() signature (so it's not like .Handle()) and keep .Group() only?

type Router interface {
http.Handler

Group(fn func(r Router)) Router
Mount(pattern string, r Router)
Handle(pattern string, h http.Handler)
//...

}

// --------

r := chi.NewRouter()
r.Use(ForAllCtx)

r.Group(func(r chi.Router) {
r.Use(SpecialCtx)
r.Get("/", handler)
})

r.Mount("/ping", subrouter)

r.Mount("/:articleID", r.Group(func(r chi.Router) {
r.Use(ArticleCtx)
r.Get("/", getArticle)
}))
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

[c2h5oh]

Good. :-)

[c2h5oh]

IMO it should be

Handle(method, pattern string, h http.Handler)
All(pattern string, h http.Handler)

First All (or Any) is more self-explanatory, second method set can be extended. Not just "can be extended in theory" but this is something that is frequently done when building a protocol on top of HTTP - see WEBDAV for example.

[c2h5oh]

Supporting this would also require a change to treeRouter.route to use string as key.

OR, which I believe is better

have a single route on treeRouter & have map[method]http.Handler on node

Benefits:

• single route tree, instead of multiple (you are adding chain to ALL method trees on .Route now)
• extensible method list
• proper HTTP 405 handling (we're returning 404 when there is a route, just not for current method, which is incorrect - 404 should be returned when there is no route for ANY method)
• no pointers in the tree -> no indirects -> routing tree as a single object and will often fit entirely in CPU cache (vs cache miss on every routing level - 2-5 CPU cycles vs 200-400 - see the very first table in https://blog.codinghorror.com/the-infinite-space-between-words/ )

, instead of having a route[method]*treeRouter in a tree

[vektah]

We have a chi based reverse proxy serving ~12M requests a day. It has a lot of http.Handler middleware so there were many layers of 'mwrap' on our requests.

I updated to golang 1.7 and this branch to see how the benchmarks would look:

golang 1.6.3 + chi 1
PASS
BenchmarkStack-4                           30000             44042 ns/op            6083 B/op        122 allocs/op
BenchmarkFullRequest-4                     20000             83779 ns/op           10046 B/op        206 allocs/op
BenchmarkFullGZIPRequest-4                 20000             84837 ns/op           10323 B/op        207 allocs/op
BenchmarkFullRequestConcurrently-4            50          23300284 ns/op         6001109 B/op      34279 allocs/op
Benchmark404PageConcurrently-4                30          50251755 ns/op        15214906 B/op      91058 allocs/op
BenchmarkGzip-4                              200           6952123 ns/op           54879 B/op        169 allocs/op

golang 1.7-rc3 + chi v2
BenchmarkStack-4                           50000             40403 ns/op            6053 B/op        104 allocs/op
BenchmarkFullRequest-4                     20000             64660 ns/op            9529 B/op        177 allocs/op
BenchmarkFullGZIPRequest-4                 20000             65922 ns/op            9802 B/op        178 allocs/op
BenchmarkFullRequestConcurrently-4           100          19811847 ns/op         6071349 B/op      32629 allocs/op
Benchmark404PageConcurrently-4                30          38976743 ns/op        15151510 B/op      89831 allocs/op
BenchmarkGzip-4                              200           6060412 ns/op           47497 B/op        169 allocs/op

Approaching +30%! I'm excited to see the real-world performance.

[vektah]

I cant see a way to mount a GET/POST/PUT only http.Handler, short of wrapping it in a:

r.Get("/foo", func(w http.ResponseWriter, r *http.Request) {
    handler.ServeHttp(w, r)
})

Perhaps these should be http.Handler?

[lwc]

Yeah seconded - there is Handle and HandleFunc, should there also be Get and GetFunc?

Targeting the interface imposes less opinions on usage.

[pkieltyka]

@vektah great to hear! chi v2 doesn't need mwrap anymore :)

http.HandlerFunc provides us the most flexibility while keeping the chi.Router interface succinct and clean. You can write:

r.Get("/foo", handler.ServeHTTP)

[vektah]

I think I'm mostly just dragging baggage from gorilla where you need to wrap middleware yourself, and HandlerFunc becomes kinda gross. Not really a problem with chi though.

Ugh, why didn't I think of just passing the func...

[VojtechVitek]

@vektah thanks for the interesting benchmark results!

[VojtechVitek]

@pkieltyka Instead of this chi photo, we should show a simple usage example or benchmarks.