ProblemDetailsService Can Simplify Writers Evaluation

[commonsensesoftware]

Is there an existing issue for this?

• I have searched the existing issues

Describe the bug

The ProblemDetailsService implementation uses the last registered IProblemDetailsWriter where CanWrite returns true as seen at:

aspnetcore/src/Http/Http.Extensions/src/ProblemDetailsService.cs

Line 42 in 5918bc9

for (var i = 0; i < _writers.Length; i++)

Although a micro-optimization, the loop should be processed in reverse. As soon as a writer on the tail end can write the ProblemDetails, then the loop can break. In fact, the entire implementation can be simplified to:

    public ValueTask WriteAsync(ProblemDetailsContext context)
    {
        ArgumentNullException.ThrowIfNull(context);
        ArgumentNullException.ThrowIfNull(context.ProblemDetails);
        ArgumentNullException.ThrowIfNull(context.HttpContext);

        if (context.HttpContext.Response.HasStarted ||
            context.HttpContext.Response.StatusCode < 400 ||
            _writers.Length == 0)
        {
            return ValueTask.CompletedTask;
        }

-        IProblemDetailsWriter? selectedWriter = null;

-        if (_writers.Length == 1)
-        {
-            selectedWriter = _writers[0];

-            return selectedWriter.CanWrite(context) ?
-                selectedWriter.WriteAsync(context) :
-                ValueTask.CompletedTask;
-        }

-       for (var i = 0; i < _writers.Length; i++)
+       for (var i = _writers.Length - 1; i >= 0; i--)
        {
+            var selectedWriter = _writers[i];
-            if (_writers[i].CanWrite(context))
+            if (selectedWriter.CanWrite(context))
-            {
-                selectedWriter = _writers[i];
-                break;
+                return selectedWriter.WriteAsync(context);
-            }
        }

-        if (selectedWriter != null)
-        {
-            return selectedWriter.WriteAsync(context);
-        }

        return ValueTask.CompletedTask;
    }

Expected Behavior

IProblemDetailsService.WriteAsync should execute with the fewest number of operations necessary.

Steps To Reproduce

var builder = WebApplication.CreateBuilder( args );

builder.Services.AddProblemDetails();

Exceptions (if any)

No response

.NET Version

7.0.100

Anything else?

Happy to submit a PR if the proposed changes are accepted.

[commonsensesoftware]

cc @brunolins16

[brunolins16]

@commonsensesoftware Sorry, I am not quite sure if I understood your point.

The ProblemDetailsService implementation uses the last registered IProblemDetailsWriter where CanWrite returns true as seen at:

The ProblemDetailsService uses the first registered IProblemDetailsWriter where CanWrite is true. Here is an unit test that shows this behavior:

aspnetcore/src/Http/Http.Extensions/test/ProblemDetailsServiceTest.cs

Line 11 in 7d3b46d

public async Task WriteAsync_Skip_NextWriters_WhenResponseAlreadyStarted()

As soon as a writer on the tail end can write the ProblemDetails, then the loop can break.

This is almost the same behavior but not in reverse. So, for example, if you have Default +Custom writer we will check custom first and break if it can write or move to default. Also, if you have only one this will evaluate without enumerating.

That was decided because the expectation is that the call to AddProblemDetails usually will happens after AddControllers or AddTransient/Singleton/etc.. to register custom writers. If we change to loop in reverse, we will almost never evaluate other registered writers.

[commonsensesoftware]

🤦🏽 Uggh.. you're so right. Duh?! That sneaky break;🥷

It could still be simplified to:

    public ValueTask WriteAsync(ProblemDetailsContext context)
    {
        ArgumentNullException.ThrowIfNull(context);
        ArgumentNullException.ThrowIfNull(context.ProblemDetails);
        ArgumentNullException.ThrowIfNull(context.HttpContext);

        if (context.HttpContext.Response.HasStarted ||
            context.HttpContext.Response.StatusCode < 400 ||
            _writers.Length == 0)
        {
            return ValueTask.CompletedTask;
        }

-        IProblemDetailsWriter? selectedWriter = null;

-        if (_writers.Length == 1)
-        {
-            selectedWriter = _writers[0];

-            return selectedWriter.CanWrite(context) ?
-                selectedWriter.WriteAsync(context) :
-                ValueTask.CompletedTask;
-        }

       for (var i = 0; i < _writers.Length; i++)
       {
+            var selectedWriter = _writers[i];
-            if (_writers[i].CanWrite(context))
+            if (selectedWriter.CanWrite(context))
-            {
-                selectedWriter = _writers[i];
-                break;
+                return selectedWriter.WriteAsync(context);
-            }
        }

-        if (selectedWriter != null)
-        {
-            return selectedWriter.WriteAsync(context);
-        }

        return ValueTask.CompletedTask;
    }

This would simply the logic a lot and remove to if statements in the worst case, which is anything where _writers.Length > 1.

We're pinching ¢¢¢ here, but I noticed it and every little bit helps. If you don't think the juice is worth the squeeze, I'm happy to close this out. If you want continue, I'll update the title because, as you've rightly pointed out, the ordering is correct.

[brunolins16]

I would keep the if (_writers.Length == 1) but the other suggestions look good. If you are interested feel free to open a PR or I can try open a quick PR later this week. Thanks for the suggestion.

[commonsensesoftware]

Sure. I can spin up a PR.

What is gained (or retained) by keeping the branch for if (_writers.Length == 1)? Since `writers supports indexing, I don't see any additional allocations or other benefits. I'm just seeing another branch that does the same thing as the loop. The net effect will be the same and is seeming unnecessary.

Just curious. If there is some performance benefit, I would like to learn about it for future reference.

[commonsensesoftware]

Updated the title to correctly reflect the proposed change

[brunolins16]

Sure. I can spin up a PR.

What is gained (or retained) by keeping the branch for if (_writers.Length == 1)? Since `writers supports indexing, I don't see any additional allocations or other benefits. I'm just seeing another branch that does the same thing as the loop. The net effect will be the same and is seeming unnecessary.

Just curious. If there is some performance benefit, I would like to learn about it for future reference.

Actually, you are right. I just noticed that writers is an array already, so, I think no extra allocations.

Thanks for contacting us.

We're moving this issue to the .NET 8 Planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s).
If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues.
To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

[captainsafia]

Triage: This is a fairly small and well-defined change. Moving it to .NET 8 Planning and adding help wanted for someone to comment.