Skip to content

Refactor HttpRuntime usage to minimize statics usage #295

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Feb 17, 2023

Conversation

twsouthwick
Copy link
Member

In ASP.NET Framework, HttpRuntime was used as a static holder for the runtime information. We expose this type in the adapters, but it should not be accessed directly internally due to its static nature. This currently causes sporadic test failures. Rather, internally should access the IHttpRuntime if this information is needed.

This change includes:

  • Changes UrlPath to be a class that takes in the IHttpRuntime
  • Moves the logic of VirtualPathUtility into a VirtualPathUtilityImpl that is an instance class
  • VirtualPathUtility uses a cached instance of VirtualPathUtilityImpl when needed
  • Current usage of VirtualPathUtility in the adapters is replaced with the VirtualPathUtilityImpl
  • Tests are updated to no longer require setting HttpRuntime.Current

In ASP.NET Framework, HttpRuntime was used as a static holder for the runtime information. We expose this type in the adapters, but it should not be accessed directly internally due to its static nature. This currently causes sporadic test failures. Rather, internally should access the IHttpRuntime if this information is needed.

This change includes:

- Changes UrlPath to be a class that takes in the IHttpRuntime
- Moves the logic of VirtualPathUtility into a VirtualPathUtilityImpl that is an instance class
- VirtualPathUtility uses a cached instance of VirtualPathUtilityImpl when needed
- Current usage of VirtualPathUtility in the adapters is replaced with the VirtualPathUtilityImpl
- Tests are updated to no longer require setting HttpRuntime.Current
@twsouthwick
Copy link
Member Author

/cc @CZEMacLeod

@twsouthwick twsouthwick requested a review from Tratcher February 17, 2023 22:37
@twsouthwick twsouthwick merged commit e8eff1d into main Feb 17, 2023
@twsouthwick twsouthwick deleted the tasou/runtime-via-di branch February 17, 2023 23:05
@CZEMacLeod
Copy link
Contributor

Just a couple of quick notes - this should probably mention that it includes a heap of changes to add copyright information to some files not associated with HttpRuntime or the VPU stuff.
Also changes Cache to match code style (no single line ifs, and removing backing fields).
I think the ref file should not have System.Runtime.CompilerServices.CompilerGeneratedAttribute in it, as this does not match the native dll, and is an implementation detail.
That attribute should maybe be added to the excluded attributes file instead. (src/Microsoft.AspNetCore.SystemWebAdapters/Generated/ExcludedAttributes.txt)

@twsouthwick
Copy link
Member Author

Created #298 to remove the attribute. Good call.

All the code is inspired or taken from referencesource so not sure we need specific call outs for that. I'll follow up on that.

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