Skip to content

add support for subtree routing#57

Closed
rogpeppe wants to merge 1 commit into
julienschmidt:masterfrom
rogpeppe-contrib:002-subrouter
Closed

add support for subtree routing#57
rogpeppe wants to merge 1 commit into
julienschmidt:masterfrom
rogpeppe-contrib:002-subrouter

Conversation

@rogpeppe
Copy link
Copy Markdown
Contributor

This makes it feasible to compose routers modularly from
sub-routers that implement independent subtrees, rather
than requiring all routes to be combined into a single
large router.

@patrickToca
Copy link
Copy Markdown

Excellent. A readMe "sub-routers use case" (complement to the test) would be useful. If have I time I will document one. Not sure when.

@julienschmidt
Copy link
Copy Markdown
Owner

Sorry, but this is not going to be merged since it basically breaks the main promises of this router.

But I agree, something like sub-routers are feasible. My current plan is to simple return a handle ("sub-router") when registering a route which can be named (#10) and used for registering sub-routes.

@julienschmidt
Copy link
Copy Markdown
Owner

Since the idea in general is great and you seems like you put some effort in it (thanks for that!) let me explain a bit further why I don't want to merge this implementation:

HttpRouter is intended as a lightweight and slim package. This also includes package API, which should be as simple as possible. Therefore, this router will also never be as powerful as gorilla/mux for example.

I think this approach is problematic because it requires manual chaining of the routers, introduces another rather confusing public method and actually slows down the router. It is convenient to use sub-routers, but the more you use it, the more overhead you introduce.
I'd prefer an approach which gives this extra convenience at no extra cost.

@rogpeppe
Copy link
Copy Markdown
Contributor Author

Thanks for your helpful response. I'll try to explain why I think that this is
actually a reasonable proposal and why I don't think your proposed
solution covers my use case. Sorry in advance for the length of this message.

The scenario I have in mind is this one: suppose I am writing a package
that serves some subtree of an HTTP namespace, but is agnostic
as to where it actually lives in the namespace.

The net/http/pprof package is a nice example of this (except that it
isn't because it doesn't actually work properly if you put it anywhere
other than /, but please ignore that for the moment).
We have quite a few examples in our current code base.

The package might look something like this:

package reusableservice
// Handler returns an httprouter handler that serves the
// relevant endpoints and understands the httprouter convention
// for serving a subpath.
func Handler() httprouter.Handle

I would like to embed a handler created with
reusableservice.Handler inside my own router. The only way I can
currently do that is by trimming the URL path before invoking
the reusableservice handler, because otherwise it doesn't
know which part of the path it is supposed to interpret.
But if we do that, then URL redirection is broken because it redirects
relative to the absolute URL path which is now incorrect.

A way around this, which I think is compatible with the solution
you suggest for subrouting above would be to change the API to do something like:

// AddRoutes adds all relevant endpoints to the given router.
func AddRoutes(r *httprouter.Router)

but this isn't nearly as nice. It precludes the package from adding its own
middleware wrapper around the handler invocation, and it becomes harder
to use other kinds of router as implementations inside reusableservice.
It's also no longer possible to correctly embed the handler inside (for example)
an http.ServeMux. This new API is not as modular as it was.

The nice thing about having a Handle is that it's composable. It
can be arbitrarily wrapped (hiding what's inside), and that's part
of the power of the Go HTTP framework. By requiring the second
solution above, we lose some of that power.

As for performance, yes, there is a performance hit of around
5ns per handle call on my machine,
because there is one more branch to be tested, but that seems like
a not unreasonable price to pay (and I'm sure it could be recouped somewhere
else if really needed)

Here's a comparative benchmark:

BenchmarkHttpRouter_Param            161           166           +3.11%
BenchmarkHttpRouter_Param5           441           442           +0.23%
BenchmarkHttpRouter_Param20          1395          1419          +1.72%
BenchmarkHttpRouter_ParamWrite       231           236           +2.16%
BenchmarkHttpRouter_GithubStatic     75.5          77.7          +2.91%
BenchmarkHttpRouter_GithubParam      376           384           +2.13%
BenchmarkHttpRouter_GithubAll        64704         65048         +0.53%
BenchmarkHttpRouter_GPlusStatic      44.5          47.3          +6.29%
BenchmarkHttpRouter_GPlusParam       250           255           +2.00%
BenchmarkHttpRouter_GPlus2Params     298           304           +2.01%
BenchmarkHttpRouter_GPlusAll         3167          3240          +2.31%
BenchmarkHttpRouter_ParseStatic      46.1          49.6          +7.59%
BenchmarkHttpRouter_ParseParam       219           223           +1.83%
BenchmarkHttpRouter_Parse2Params     249           256           +2.81%
BenchmarkHttpRouter_ParseAll         4566          4693          +2.78%
BenchmarkHttpRouter_StaticAll        18729         18818         +0.48%

There will of course be some performance overhead from using embedded routers
like this, and if you use them a lot, your performance will drop. I measure about 200ns
and one allocation overhead per extra level. For many use cases, I contend
that is a perfectly OK performance hit to take for the added modularity - the
user can of course make that choice.

As for the new public method, I agree that the name is not ideal, but is
it actually unreasonable to have httprouter implement its own Handle method type?

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