Skip to content

Update C# code for #29448 #56086

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.

Sign up for GitHub

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

Jump to bottom
Open
paulomorgado wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Update C# code for #29448 #56086

paulomorgado wants to merge 9 commits into from
+59 −61

Conversation

@paulomorgado
Copy link
Contributor

@paulomorgado paulomorgado commented Jun 5, 2024

Update C# code for #29448

  • You've read the Contributor Guide and Code of Conduct.
  • You've included unit or integration tests for your change, where applicable.
  • You've included inline docs for your change, where applicable.
  • There's an open issue for the PR that you are making. If you'd like to propose a new feature or change, please open an issue to discuss the change or find an existing issue.

Summary of the changes (Less than 80 chars)

Description

When #29448 was merged, there were no static lambdas in C# and AOT was not a thing.

A new struct UriComponents has been introduced to encapsulate the components of a URI: Scheme, Host, PathBase, Path, Query, and Fragment to avoid a dependency on ValueTuple{6}. The string.Create method call has been updated to use a new lambda function that leverages the UriComponents struct to copy each component of the URI into the buffer, adjusting the buffer slice as needed. This function also handles the case where both PathBase and Path components have a slash, removing the trailing slash from PathBase to avoid duplication.

Finally, the BuildAbsolute method has been updated to use the new UriComponents struct and the updated string.Create call.

Related to #29448 and #28905

@ghost ghost added the area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions label Jun 5, 2024
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Jun 5, 2024
@paulomorgado paulomorgado force-pushed the paulomorgado/issue-28905 branch from 357eca4 to 2c7a6f8 Compare June 5, 2024 17:02
@amcasey
Copy link
Member

amcasey commented Jun 5, 2024

CI failures look legit. Probably need some #ifs.

@BrennanConroy
Copy link
Member

BrennanConroy commented Jun 5, 2024

A new struct UriComponents has been introduced to encapsulate the components of a URI: Scheme, Host, PathBase, Path, Query, and Fragment to avoid a dependency on ValueTuple{6}.

I'm not an AOT expert, but isn't creating a new struct going to do basically the same thing in AOT as depending on ValueTuple{6}?

@paulomorgado
Copy link
Contributor Author

paulomorgado commented Jun 7, 2024 •
edited
Loading

@BrennanConroy,

A new struct UriComponents has been introduced to encapsulate the components of a URI: Scheme, Host, PathBase, Path, Query, and Fragment to avoid a dependency on ValueTuple{6}.

I'm not an AOT expert, but isn't creating a new struct going to do basically the same thing in AOT as depending on ValueTuple{6}?

From @stephentoub on #55611 (comment):

And it's going to contribute to a larger NativeAOT footprint, due to the use of the ValueTuple`5, which may not be otherwise used in the app and which brings with it a non-trivial footprint.

@stephentoub
Copy link
Member

stephentoub commented Jun 7, 2024

ValueTuple implements multiple interfaces and as a result contains a lot of code that generally can't be trimmed. If it's not otherwise used in the app, it's unnecessary bloat. At the app level, not a big deal, but at the runtime level, we pay attention to such things.

@paulomorgado
Copy link
Contributor Author

paulomorgado commented Jun 7, 2024 •
edited
Loading

@amcasey,

CI failures look legit. Probably need some #ifs.

There was a name collision between System.UriComponent and Microsoft.AspNetCore.Http.Extensions.UriHelper+UriComponent (introduced in this PR) that I have solved.

The other errors look to me as environmental and not related to this change. Am I missing anything?

@amcasey
Copy link
Member

amcasey commented Jun 7, 2024

@amcasey,

CI failures look legit. Probably need some #ifs.

There was a name collision between System.UriComponent and Microsoft.AspNetCore.Http.Extensions.UriHelper+UriComponent (introduced in this PR) that I have solved.

The other errors look to me as environmental and not related to this change. Am I missing anything?

Nope, I was referring to the name collision. The new failure is probably unrelated, though it's not one I've seen before. A nuget restore failure could have been a transient network issue?

@dotnet-policy-service dotnet-policy-service bot added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Jun 14, 2024
@paulomorgado
Copy link
Contributor Author

paulomorgado commented Jun 14, 2024

@amcasey, all builds seem to have succeeded.

@amcasey
Copy link
Member

amcasey commented Jun 14, 2024

All good, @BrennanConroy?

@paulomorgado
Copy link
Contributor Author

paulomorgado commented Aug 28, 2024

Hi @adityamandaleeka,

Anything blocking this PR?

@paulomorgado
Refactor UriHelper.cs to use a static lambda and UriComponents struct
65b6834 
This commit includes a significant refactor of the `UriHelper.cs` file. Unused namespaces `System.Buffers` and `System.Runtime.CompilerServices` have been removed. The `InitializeAbsoluteUriStringSpanAction` delegate and `InitializeAbsoluteUriString` method, as well as the `CopyTextToBuffer` method, have been removed.

A new struct `UriComponents` has been introduced to encapsulate the components of a URI: `Scheme`, `Host`, `PathBase`, `Path`, `Query`, and `Fragment`. The `string.Create` method call has been updated to use a new lambda function that leverages the `UriComponents` struct to copy each component of the URI into the buffer, adjusting the buffer slice as needed. This function also handles the case where both `PathBase` and `Path` components have a slash, removing the trailing slash from `PathBase` to avoid duplication.

Finally, the `BuildAbsolute` method has been updated to use the new `UriComponents` struct and the updated `string.Create` call.
@paulomorgado
fixed "src/Http/Http.Extensions/src/UriHelper.cs(205,52): error CS0117"
67f8239
@paulomorgado paulomorgado force-pushed the paulomorgado/issue-28905 branch from ee9a23a to 67f8239 Compare November 15, 2024 17:23
BrennanConroy
BrennanConroy approved these changes Dec 9, 2025
View reviewed changes
Copy link
Member

@BrennanConroy BrennanConroy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if making UriComponents a ref struct would be beneficial here.

Copilot AI review requested due to automatic review settings December 10, 2025 00:04
Copilot AI reviewed Dec 10, 2025
View reviewed changes
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR modernizes the UriHelper.BuildAbsolute method to improve AOT (Ahead-of-Time) compilation compatibility by replacing a SpanAction delegate with a static lambda and introducing a custom UriComponents struct. This change eliminates the dependency on System.Buffers and avoids using ValueTuple, both of which can cause issues with AOT scenarios.

Key changes:

  • Replaced SpanAction<char, ValueTuple<...>> delegate with a static lambda for better AOT compatibility
  • Introduced UriComponents readonly struct to encapsulate URI components instead of using ValueTuple
  • Removed unused System.Buffers using directive and related static field

BrennanConroy
BrennanConroy reviewed Dec 10, 2025
View reviewed changes
@build-analysis build-analysis bot mentioned this pull request Dec 10, 2025
Open
paulomorgado and others added 3 commits December 10, 2025 11:06
@paulomorgado
Update src/Http/Http.Extensions/src/UriHelper.cs
18f15ff 
Co-authored-by: Copilot <[email protected]>
@paulomorgado
Update src/Http/Http.Extensions/src/UriHelper.cs
e5c8be3 
Co-authored-by: Copilot <[email protected]>
@paulomorgado
Change UriComponents to readonly ref struct
dc1ffa0 
Refactored UriComponents from a regular readonly struct to a readonly ref struct to enforce stack-only allocation and prevent boxing or heap storage. This enhances safety and performance for scenarios where UriComponents is used.
@paulomorgado
Copy link
Contributor Author

paulomorgado commented Dec 10, 2025

Hi @BrennanConroy,

I wonder if making UriComponents a ref struct would be beneficial here.

I pushed that change and the build seems to have succeeded, although, locally, I'm getting a "CS9244: The type may not be a ref struct or a type parameter allowing ref structs in order to use it as parameter in the generic type or method."

Being a readonly ref struct, we could use ReadOnlySpan<char> instead of string.

@paulomorgado
Refactor UriHelper to use ReadOnlySpan for components
6dd20ee 
Switch URI component handling from string to ReadOnlySpan<char> to reduce allocations and improve performance. Update UriComponents struct and string.Create logic to operate directly on spans, and move pathBase trailing slash trimming to span operations.
@BrennanConroy
Copy link
Member

BrennanConroy commented Dec 16, 2025

Let's revert that, it was just a thought.

@paulomorgado
Copy link
Contributor Author

paulomorgado commented Dec 17, 2025

Let's revert that, it was just a thought.

Why? It seems to be working fine, and it makes the code simpler.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@BrennanConroy BrennanConroy BrennanConroy approved these changes

Copilot code review Copilot Copilot left review comments

@halter73 halter73 Awaiting requested review from halter73 halter73 is a code owner

@captainsafia captainsafia Awaiting requested review from captainsafia captainsafia is a code owner

+1 more reviewer

@mkArtakMSFT mkArtakMSFT mkArtakMSFT approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@BrennanConroy BrennanConroy

Labels

area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions community-contribution Indicates that the PR has been added by a community member pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@paulomorgado @amcasey @BrennanConroy @stephentoub @mkArtakMSFT @adityamandaleeka