Fix #6102 - Intense CPU utilization on page change (#6542)

[rynowak]

Description

#6543

The routing system in ASP.NET Core supports changing when a Razor Page is added, removed, or edited. 2.2.0 has a bug where the address table in Endpoint Routing subscribes to route table change notifications an additional time each time a change occurs. This means that each route table change is processed N additional times when it occurs - where N is the number of changes so far.

Customer Impact

Developers can see high CPU and memory utilization when they make a change that triggers rebuilding of the route table (edit a Razor Page while the site is running). This will escalate over time as more changes are made until the application becomes unusable. Customers will primarily hit this in inner-loop development, but this can also happen in production. We know that many developers like to have recompilation available in production.

Regression?

Yes, this is a bug in a new 2.2 feature.

Risk

Low. The fix is to remove the offending line of code. Additional steps are taken to harden the code, and add IDisposable support so that infrastructure pieces unsubscribe from change notifications when disposed.

Since these changes are infrastructure, there is no impact in the user-visible behavior.

---

The issue here was that every time a Razor Page changed, we would
subscribe an additional time to the endpoint change notifications. This
means that if you tweaked a page 30 times, we would update the address
table 31 times when you save the file. If you were doing a lot of editing
then this would grow to a really large amount of computation.

The fix is to use DataSourceDependentCache, which is an existing utility
type we developed for this purpose. I'm not sure why it wasn't being
used for this already. We're already using DataSourceDependentCache in a
bunch of other places, and it's well tested.

I also tweaked the stucture of this code to be more similar to
EndpointNameAddressScheme. This involved some test changes that all
seemed like good cleanup. The way this was being tested was a little
wonky.

(cherry picked from commit a5658a8)

[rynowak]

This is a straightforward cherry-pick of the fix from master.

[JamesNK]

You need to put your description into the ship room template and tag with servicing consider 😋

e.g. #6645

[rynowak]

Updated

[mkArtakMSFT]

Just couple of notes to consider.

[natemcmaster]

Can you please update eng/PatchConfig.props to list the packages which need to ship an update?

[leecow]

Approved. Merge soon ;-)

[rynowak]

@natemcmaster @mikaelm12 @dougbu - could one of you review the last commit?

[dougbu]

@rynowak I'm surprised you don't have merge conflicts because the 2.2.2 section already exists and contains a few rows. The changes otherwise look fine.

[mikaelm12]

I'm surprised you don't have merge conflicts because the 2.2.2 section already exists and contains a few rows.

Yeah, I figured you would need to rebase. Not sure why github isn't reporting a conflict there since there definitely are changes in the PatchConfig.props in the 2.2 branch.

[JamesNK]

A patch PR of mine will add Microsoft.AspNetCore.Routing to the file.

[rynowak]

I was also surprised not to see it. Should I rebase?

[mikaelm12]

James already added the package to the PatchConfig.props so you're change to it is no longer showing up. You could rebase to be safe but I don't think it's necessary.