Multiple calls to UseRouting breaks endpoint routing

khellang commented 4 years ago

If you have multiple calls to UseRouting, you won't be able to pull the resolved endpoint between the two calls:

This is because the latter overrides the DefaultEndpointRouteBuilder set by the former call:

https://github.com/aspnet/AspNetCore/blob/33055d9a3c3a2a296dcd0152ccfe0df04b6d2109/src/Http/Routing/src/Builder/EndpointRoutingApplicationBuilderExtensions.cs#L44-L45

I think the UseRouting method should check whether the EndpointRouteBuilder key already exists and noop if it does, possibly also log a warning. The EndpointRoutingMiddleware already short-circuits in the case of multiple instances in the pipeline, so subsequent instances are effectively noops already:

https://github.com/aspnet/AspNetCore/blob/33055d9a3c3a2a296dcd0152ccfe0df04b6d2109/src/Http/Routing/src/EndpointRoutingMiddleware.cs#L49-L55

// @rynowak @khalidabuhakmeh

javiercn commented 4 years ago

@khellang thanks for contacting us.

@rynowak Any thoughts?

khellang commented 4 years ago

In addition to the runtime checks, we could consider adding an analyzer for flagging redundant calls to UseRouting.

khalidabuhakmeh commented 4 years ago

Not sure an analyzer is helpful, since the issue may likely be in third party libraries making the calls internally.

khellang commented 4 years ago

Not sure an analyzer is helpful, since the issue may likely be in third party libraries making the calls internally.

It could be helpful

In addition to the runtime checks

There's already several of them;

https://github.com/aspnet/AspNetCore/blob/master/src/Analyzers/Analyzers/src/StartupAnalyzer.cs#L83-L85

billbogaiv commented 4 years ago

I'd much prefer if the no-op logic was in the app.UseRouting() extension. Alternatively, at-least make https://github.com/aspnet/AspNetCore/blob/33055d9a3c3a2a296dcd0152ccfe0df04b6d2109/src/Http/Routing/src/Builder/EndpointRoutingApplicationBuilderExtensions.cs#L14 public so libraries that call app.UseRouting() could check whether the builder exists without having to worry about dictionary key-changes in future versions.

I'm still a bit lost why the sample doesn't work, but this does:

app
    .UseRouting()
    .Use(async (context, next) =>
    {
        var endpoint = context.GetEndpoint(); // null

        await next();
    })
    .UseRouting()
    .Use(async (context, next) =>
    {
        var endpoint = context.GetEndpoint(); // not null

        await next();
    })
    .UseEndpoints(endpoints =>
    {
        endpoints.MapRazorPages();
    });

khellang commented 4 years ago

I'd much prefer if the no-op logic was in the app.UseRouting() extension.

Yeah, that's what I'm saying. This would be my preferred solution; https://github.com/aspnet/AspNetCore/commit/19bcf5e25604ca077efbb8068d2402a872fe68b6, in addition to an analyzer.

I'm still a bit lost why the sample doesn't work, but this does:

Because the second call to UseRouting overwrites the builder, the call to UseEndpoints never sees the builder from the first call to UseRouting, which means MapRazorPages never makes it into the endpoint source for the first EndpointRoutingMiddleware, which means it's unable to resolve the endpoint.

billbogaiv commented 4 years ago

So, builder.Properties[EndpointRouteBuilder] getting assigned the same endpointRouteBuilder-instance that gets passed to EndpointRoutingMiddleware is critical in this case? That's the part I got stuck at when initially going through this problem with @khalidabuhakmeh.

khellang commented 4 years ago

So, builder.Properties[EndpointRouteBuilder] getting assigned the same endpointRouteBuilder-instance that gets passed to EndpointRoutingMiddleware is critical in this case?

Yes, the first middleware gets one builder instance, which gets overwritten (thrown away) immediately. The second middleware gets a new builder instance, which is the one being picked up by UseEndpoints and filled with endpoints.

rynowak commented 4 years ago

Can I get a high level of what people are doing that hits the problem? This would help a lot in justifying what we should do about it 😄

Is this just a mistake someone made?
Is there legit scenario someone is trying to do that gets blocked by this?

To add some context to this, the reason why the current behavior is the way it is so that we can behave reasonably in some wierdo cases.

I think the UseRouting method should check whether the EndpointRouteBuilder key already exists and noop if it does, possibly also log a warning. The EndpointRoutingMiddleware already short-circuits in the case of multiple instances in the pipeline, so subsequent instances are effectively noops already:

In my experience logging in ASP.NET Core isn't a helpful solution to help users find possible programming mistakes.

If it's not a mistake then the logging is always going to appear
If it is a mistake then most people won't notice the logging (hard to see in VS)

khellang commented 4 years ago

Can I get a high level of what people are doing that hits the problem?

I have a really hard time seeing this could be anything but a mistake. The way I see it, there's nothing to gain from this with the current behavior, as the routing middleware already short-circuits if you have multiple instances.

In my experience logging in ASP.NET Core isn't a helpful solution to help users find possible programming mistakes.

What makes you say this? Is there precedent here? It's weird, 'cause logging warnings for obvious mistakes and upcoming API changes has worked amazingly for frameworks like React etc. If you can't rely on logging being seen, what can you rely on? Design-time analyzers and runtime exceptions only?

Anyway, if you're not stoked on logging, I think we're left with three options:

Just close this issue and say "don't do that".
Start throwing exceptions if UseRouting has already been called.
Let subsequent calls to UseRouting "fail" in silence (at runtime) and add an analyzer that warns you that multiple calls to UseRouting has no effect.

IMO, all middleware should consider the following scenarios:

"What happens if services I require haven't been registered?" (VerifyRoutingServicesAreRegistered)
"What happens if I'm added to the pipeline out of a specific order?" (VerifyEndpointRoutingMiddlewareIsRegistered + analyzer)
"What happens if I'm added to the pipeline multiple times?" (this is where UseRouting fails)

Because of that, I think 1. would be a cop-out and not really helpful.

Throwing would probably be best, if it wasn't a breaking change. People can happily have multiple calls to UseRouting in their applications today, perhaps not even knowing about it (because of some esoteric framework or something), as long as they don't attempt to access an endpoint before the last call to UseRouting. Then suddenly their app breaks at startup because UseRouting starts throwing.

That leaves us with 3. Fortunately, IMO, we're able to Do The Right Thing™ by not adding the middleware and not overwriting the builder. That will fix stuff that didn't work before, and avoid breaking stuff that did work before.

I don't think we need to answer the question "why did the developer try to call UseRouting multiple times?" now. If someone deliberately wants to call UseRouting multiple times to do something that's not possible today, they can open up an issue to explain what they're trying to do.

rynowak commented 4 years ago

I have a really hard time seeing this could be anything but a mistake. The way I see it, there's nothing to gain from this with the current behavior, as the routing middleware already short-circuits if you have multiple instances.

Cool, I'd like to hear from @khalidabuhakmeh and @billbogaiv as well.

What makes you say this? Is there precedent here? It's weird, 'cause logging warnings for obvious mistakes and upcoming API changes has worked amazingly for frameworks like React etc. If you can't rely on logging being seen, what can you rely on?

Right, we don't rely on logging being seen for people doing local development.

Because of that, I think 1. would be a cop-out and not really helpful.

There's lots of things we choose not to do something about. Writing analyzers has a cost to write and maintain them. Making changes to runtime behavior has a risk of breaking legitimate scenarios and limiting future (currently unanticipated scenarios).

My preference would be to change the runtime behavior to be less surprising - provided that we can find a clear right thing to do. We can make breaking changes if necessary. Without getting the whole picture from the people who wanted to discuss, then the default option is to wait until we get more feedback.

khalidabuhakmeh commented 4 years ago

Hello @rynowak

The code at the top of this issue was more of a reproduction of the problem rather than what our code was actually doing.

The problem arose when we had a shared package that does common bootstrapping tasks for our ASP.NET Core applications. That shared package was calling UseRouting while the application utilizing the package was also calling UseRouting. This lead to two calls to UseRouting which silently broke our middleware pipeline.

Writing analyzers has a cost to write and maintain them.

I agree with you @rynowak. I don't believe an analyzer would have helped us in this situation since the application code was only calling UseRouting once, and the external library was also calling UseRouting.

In general, analyzers help you fix your code when you've done the wrong thing. I'd prefer not to do the wrong thing in the first place ;)

My preference would be to change the runtime behavior to be less surprising

Yes, I would also agree here. In our case, we spent some time diagnosing the issue and it wasn't immediately clear what we were doing wrong. I think @khellang is on to the right solution:

Make the call to UseRouting idempotent (or fail silently)
Blow up spectacularly at runtime with a clear exception that UseRouting was called already

I'd prefer making the call idempotent, since in some cases library authors may call this method trying to be helpful for package consumers. It also shouldn't be a breaking change.

The second is also acceptable and will have to be relayed to middleware authors that they just need to check if UseRouting was called and fail if it wasn't and not try to add it.

Thank you for considering this issue either way.

rynowak commented 4 years ago

The problem arose when we had a shared package that does common bootstrapping tasks for our ASP.NET Core applications. That shared package was calling UseRouting while the application utilizing the package was also calling UseRouting. This lead to two calls to UseRouting which silently broke our middleware pipeline

Thanks for sharing your feedback. I'm relieved that this was just a bump in the road for you to figure out and not something more difficult to reason about.

My general thoughts on routing + libraries in case it's useful to you...

I'd really strongly recommend not trying to hide the middleware pipeline in a library unless you're hiding all of it. The team spent a lot of time prior to 3.0 trying to make UseRouting() OR UseEndpoints() not required - we probably evaluated 5-6 different designs. They all had problems, or caused confusion.

The challenge is that you will hit scenarios where you want to put middleware:

Before UseRouting
Between UseRouting and UseEndpoints.

Libraries that want to provide routable functionality should plug into UseEndpoints(...) rather than trying to wrap routing.

khalidabuhakmeh commented 4 years ago

@rynowak Thank you for the feedback. I think we, @billbogaiv and I, learned your advice the hard way. I wonder how other middleware authors will learn from our mistakes? I think it is important to make development mistakes quickly discoverable.

khellang commented 4 years ago

I'm not sure how this is a 'discussion' and not a bug? 🤔

rynowak commented 4 years ago

Because we've been discussing?

Libraries that want to provide routable functionality should plug into UseEndpoints(...) rather than trying to wrap routing.

After reading through this thread, I'm here ^^^^ - our docs include as well. I strongly discourage anyone from writing libraries that try to hide the use of routing from the user as an implementation detail.

Do you think that's there's still something to do here? We haven't seen more reports of this problem, or requests for clarification, more guardrails, etc.

khellang commented 4 years ago

I strongly discourage anyone from writing libraries that try to hide the use of routing from the user as an implementation detail.

Agreed.

Do you think that's there's still something to do here? We haven't seen more reports of this problem, or requests for clarification, more guardrails, etc.

It might not be a big problem, but there's still nothing stopping you from trying to call UseEndpoints multiple times, which obviously doesn't work the way you'd expect. I still think it would be nice to either

Make it work
Fail fast to avoid spending valuable time trying to figure out why it doesn't work.

Something like https://github.com/dotnet/aspnetcore/commit/19bcf5e25604ca077efbb8068d2402a872fe68b6 would be enough IMO.

If none of those options are viable, I think we should just close this issue with the current guidance of "don't do that" 😉

khalidabuhakmeh commented 4 years ago

Closing this issue is fine.

javiercn commented 4 years ago

Closing for now, the guidance is to not call it multiple times. If this resurfaces we can reconsider.

dotnet / aspnetcore

Multiple calls to UseRouting breaks endpoint routing #17210