ProblemDetailsService Can Simplify Writers Evaluation

[commonsensesoftware]

trafficstars

Is there an existing issue for this?

• [X] I have searched the existing issues

Describe the bug

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

https://github.com/dotnet/aspnetcore/blob/5918bc9db2099f90cc6f5b5db43a8d16f5dc89df/src/Http/Http.Extensions/src/ProblemDetailsService.cs#L42

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:

https://github.com/dotnet/aspnetcore/blob/7d3b46d330e64775f82abf4b1c86190ecf51d64a/src/Http/Http.Extensions/test/ProblemDetailsServiceTest.cs#L11

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.

[ghost]

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.

[commonsensesoftware]

@brunolins16 I just got a chance to revisit this. I was about to spin up the PR, but it looks like it has already been done and more so. 😄 I just want to confirm that indeed everything from our discussion was, in fact, implemented by you. I guess you didn't link or auto close it with your PR.

@captainsafia after @brunolins16 confirms, I believe we can close this issue out as Merged. That means it will land in .NET 8 if you need/want to track it. From the commit dates, I presume this will go out in Preview 2.

[brunolins16]

@commonsensesoftware that is right. I ended up implementing everything as part of another work I did. Sorry if you spent time in this PR.