What to do with web svc Router endpoint defintion fallibility

[GlenDC]

In

rama/rama-http/src/service/web/router.rs

Lines 530 to 533 in 560c2e3

	#[allow(clippy::expect_used, reason = "TODO later")]
	self.routes
	.insert(path, vec![(matcher, service)])
	.expect("add route");

we currently panic in case we fail insertin an endpoint svc into the router. As far as I know this can fail in two cases:

1. duplicate insertion (for the path)
2. invalid path

AFAIK the duplication should not be happening as we check if it exists and if so push if it does and only if it doesn't we do insert. Not perfect, but at least that should normally prevent that. The invalid path however could still happen.

Some frameworks like Axum panic for this and many other reasons as they assume that's fine given it will happen when you start the app and so immediately caught. However:

1. what if you create the router in a spawned thread? Than that thread will fail silentely (unless you join) making your process with one (router) server less
2. what if you create the router not at startup time but dynamically at runtime during any point of the lifetime other than startup?

In all these cases it's a bit crazy to panic... In my opinion panics should only be reserved for very bad stuff like OOM or loco invalid unrecoverable nuclear disaster states. Certainly not for oops I failed to create a router dynamically at runtime...

Even with this context given that doesn't make the solution easier... What to do with this?

• should we just make all methods fallible (e.g. try_get instead of get as all of them can fail?
• should we instead make it a builder like Request::builder and internally work with a Result so that you only have to deal with the error once? More ergonomic but perhaps a bit weird as you can't really act on that kind of error and it's like a fail all or nothing force move...
• work with a more awkward API, perhaps even having to fork the underlying lib so that we can prevent this all together somehow? Or perhaps force people to give verified paths that cannot fail, fine when at runtime, and for static definitions we could provide macro?
• ignore failed inserts? Probably not... seems like a very bad thing to do

None of these options are ideal and perhaps the is another option... We need to fix this before we ship 0.3 as it will be a big breaking change of the router API regardless of the option we choose.

[GlenDC]

Please give this issue a thought this week @soundofspace

[soundofspace]

Will give this some more though, but these things immediately pop in my mind when reading your description:

• I agree that this should not panic, but instead error (or prevent mistakes in some other way)
• Builders that propagate errors to final build are indeed ergonomic, but they are weird to use and also to handle errors. So would probably avoid this. The main benefit I see for this method is for for when you want to map_err to a specific one, and dont want to do it for each step of the builder, but I'm sure we can do that without forcing this pattern
• Ignoring is something I would never do, it's one of the reasons I kinda like panics, most of the time it's really in your face. This is mostly still true with errors, but in general I prefer fixing bug instead of hiding them, but there is a trade off between crashing production instances, and here Errors seem like the perfect middle ground
• we could provide a macro: maybe, but in general I tend to really enjoy macro-less api's, they are mostly easier to use and understand when you look at source code. There are lots of valid use cases for macros, but here I would avoid it, unless there is clear use case for it (that is simple and isolated enough)