Route tracing #2167
Route tracing #2167
Conversation
the10thWiz
force-pushed
the
route-tracing
branch
2 times, most recently
from
f0ccdbe
to
1400626
Compare
|
It looks like the issue with Windows Debug is a memory or hard drive error. |
|
Indeed, we're running out of disk space. Let's disregard it for now. |
the10thWiz
force-pushed
the
route-tracing
branch
3 times, most recently
from
756e34a
to
140d1fa
Compare
|
I think I've reached an API I'm pretty happy with. Since the type tracking is done internally, this could be extended to support listing the routes attempted in order, but I don't think that's necessary for this PR. I'm also happy enough with the syntax presented here, I don't feel the need to introduce a new assert macro. @SergioBenitez, this PR is ready for a review, whenever you get a chance. |
| @@ -127,6 +134,9 @@ pub struct Catcher { | |||
| /// | |||
| /// This is -(number of nonempty segments in base). | |||
| pub(crate) rank: isize, | |||
|
|
|||
| /// A unique route type to identify this route | |||
| pub(crate) catcher_type: Option<TypeId>, | |||
There was a problem hiding this comment.
Very cool! I had to read about TypeId.
- Adds RouteType and CatcherType traits to identify routes and catchers - RouteType and CatcherType are implemented via codegen for attribute macros - Adds routed_by and caught_by methods to local client response - Adds catcher to RequestState - Updates route in RequestState to None if a catcher is run - examples/hello tests now also check which route generated the reponse - Adds DefaultCatcher type to represent Rocket's default catcher - FileServer now implements RouteType
- Add with_type to catcher - Documents CatcherType, RouteType, and the associated methods in LocalResponse.
For some reason, the scripts/test.sh does not successfully run all tests - it terminates on the doctest step with a sigkill. This means I have to push changes to github to test them.
SergioBenitez
force-pushed
the
route-tracing
branch
from
fe0badf
to
09ebdbd
Compare
|
I've rebased on the latest master and push to your fork so that I can review. |
There was a problem hiding this comment.
The way I'm understanding this PR is as making fundamentally two changes:
-
Associate routes and catchers with an ID. Here, the ID is of type
TypeIdwhich is indirect obtained via new traitsCatcherTypeandRouteType, both of which requireAny. The new traits exist so that we can write conditionals of the formresponse.was_routed_by::<super::world>(), wheresuper::worldis a function annotated with#[route]. -
Before a route handler or catcher is called, set state in
Requestindicating the route or catcher. The state can then be queried to check which route or catcher actually handled the request.
On 1: I wish we could simple generate a unique ID whenever Route::new() and Catcher::new() are called and use that instead of introducing a trait that requires manual implementation for custom routes/catchers, and where failing to implement the trait doesn't result in a compile-time error but inhibits functionality. I would really like to be able to avoid these drawbacks while still being able to write something like response.was_routed_by::<super::world>().
One idea is to convert Route and Catcher into traits. Not only would this resolve the above, but it would also obviate the need for several namespace hacks we have today and pave a way to interact with code generated routes, something sorely missing today. I'm not sure how this approach changes everything else, in particular what custom routes will look like, but it's worth considering.
On 2: I'm concerned about the myriad set() calls sprinkled about. Is there a way to centralize these calls so we can't forget them?
| /// This method is marked as `cfg(test)`, and is therefore only available in unit and | ||
| /// integration tests. This is because the list of routes attempted is only collected in these | ||
| /// testing environments, to minimize performance impacts during normal operation. | ||
| #[cfg(test)] |
There was a problem hiding this comment.
This is unfortunately not the way cfg(test) works. cfg(test) only gets enabled on the top-level crate being tested, not on its dependencies. So rocket compiled as a dependency of an application, even when compiling that application's tests, will never see this cfg enabled.
Adds support for testing which route generated a response. This is an implementation of the ideas I put forward in #1878, with a few minor changes. First, I did not create a macro, since my implementation works just fine with a generic type parameter. Writing a macro is pretty trivial, if it's desired. I also added tests to the hello world example to demonstrate what the actual tests look like.
There are a few improvements that could be made:
LocalResponse(routed_by,caught_byandwas_caught) could probably have better names.TypeIdto actually use this to produce meaningful information.The only regression I see is that
Catchernow has a private member, so other crates cannot directly construct aCatcher, but I think this is acceptable, since it should probably be constructed with the constructor as before.