Move BadHttpRequestException to Http.Abstractions #20339

cado1982 · 2020-03-30T23:45:55Z

Summary of the changes (Less than 80 chars)

Replace Kestrel and IIS versions of BadHttpRequestException with shared version in Http Abstractions

Addresses #11208

This is my first contribution so I'm open to any feedback you may have.

dnfclas · 2020-03-30T23:47:17Z

All CLA requirements met.

Tratcher · 2020-03-31T17:45:36Z

@halter73 @anurse #15038 proposed making the old BadHttpRequestException derive from the new one to limit the breaking change impact. The servers would continue throwing the old ones for now.

#17804 also proposed making the Reason enum public for more granular error management.

These had both been resolved as duplicates of #11208 but the context didn't get pulled over.

src/Servers/Kestrel/Core/src/BadHttpRequestException.cs

src/Servers/Kestrel/Core/src/KestrelBadHttpRequestException.cs

analogrelay · 2020-03-31T18:49:51Z

I reopened the Reason one, we can do that in a separate PR.

cado1982 · 2020-03-31T19:45:03Z

@halter73 Let me recap what I think I need to do. Correct me if any points are wrong.

Keep the newly created Microsoft.AspNetCore.Http.BadHttpRequestException but it won't be sealed now
Make IIS and Kestrel's BadHttpRequestException derive from Microsoft.AspNetCore.Http.BadHttpRequestException
Remove the Obsolete attributes from IIS and Kestrel's old BadHttpRequestException, they'll still be used as they are now
Move all of the internal members from Kestrel's old BadHttpRequestException and have them in KestrelBadHttpRequestException instead which will derive from Kestrel's old BadHttpRequestException

I also have a question about AllowedHeader. That is currently only used in the following two places

aspnetcore/src/Servers/Kestrel/Core/src/Internal/Http/Http1Connection.cs

Line 427 in d1d9b97

BadHttpRequestException.Throw(RequestRejectionReason.ConnectMethodRequired);

aspnetcore/src/Servers/Kestrel/Core/src/Internal/Http/Http1Connection.cs

Line 471 in d1d9b97

BadHttpRequestException.Throw(RequestRejectionReason.OptionsMethodRequired);

The property is then used to set the http header here

aspnetcore/src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.cs

Line 1263 in d1d9b97

HttpResponseHeaders.HeaderAllow = ex.AllowedHeader;

Can the response header be set in the place the exceptions are thrown in Http1Connection or is it too early in the request pipeline?

halter73 · 2020-03-31T20:13:43Z

@cado1982 Looks correct except one thing:

Remove the Obsolete attributes from IIS and Kestrel's old BadHttpRequestException, they'll still be used as they are now

Keep obsoleting IIS's and Kestrel's BadHttpRequestException. You'll probably need add some pragmas to silence warnings about our own internal usage, but we want to encourage customers to catch the base type. Eventually we might remove the obsoleted derived types or make them internal.

…s version - Mark existing BadHttpRequestException in IIS and Kestrel Obsolete - Add pragmas to ignore internal Obsolete usage

cado1982 · 2020-04-01T14:10:38Z

@halter73 @anurse @Tratcher I'm not sure about this failing build. Did I break something?

Tratcher · 2020-04-01T16:30:33Z

@halter73 @anurse @Tratcher I'm not sure about this failing build. Did I break something?

No, looks like a networking issue. I've re-run it.

Tratcher

Looks pretty good. Most my comments are around reducing the need for pragmas.

src/Servers/IIS/IIS/src/BadHttpRequestException.cs

Tratcher · 2020-04-01T20:21:50Z

src/Servers/IIS/IIS/src/Core/IISHttpContext.IO.cs

@@ -122,7 +122,9 @@ private async Task ReadBody()

                    if (_consumedBytes > MaxRequestBodySize)
                    {
+#pragma warning disable CS0618 // Type or member is obsolete
                        BadHttpRequestException.Throw(RequestRejectionReason.RequestBodyTooLarge);


Lets move the static Throw API to a non obsolete internal type so we don't have to use pragma everywhere. I know that's what you were trying to do with the KestrelBadHttpRequestException, but I think we only need to move the static API.

Tratcher · 2020-04-01T20:23:01Z

src/Servers/IIS/IIS/src/Core/IISHttpContext.Log.cs

@@ -38,7 +38,9 @@ public static void UnexpectedError(ILogger logger, string className, Exception e
                _unexpectedError(logger, className, methodName, ex);
            }

+#pragma warning disable CS0618 // Type or member is obsolete
            public static void ConnectionBadRequest(ILogger logger, string connectionId, BadHttpRequestException ex)


Most of these usages can switch to referencing the base type either inline or with a using statement in the file. That will avoid excess pragmas. The two places we need to directly reference the old types are when it's first created & thrown, as well as tests that check which type was thrown. All of the other catch blocks and fields can use the new base type.

src/Http/Http.Abstractions/src/BadHttpRequestException.cs

src/Servers/Kestrel/Core/src/BadHttpRequestException.cs

Tratcher · 2020-04-01T20:49:22Z

src/Servers/Kestrel/Core/src/Internal/Http2/Http2Connection.cs

@@ -150,7 +150,9 @@ public void StopProcessingNextRequest()

        public void HandleRequestHeadersTimeout()
        {
+#pragma warning disable CS0618 // Type or member is obsolete
            Log.ConnectionBadRequest(ConnectionId, BadHttpRequestException.GetException(RequestRejectionReason.RequestHeadersTimeout));


Ah, GetException should move to a new static type as well.

Tratcher · 2020-04-01T20:50:53Z

src/Servers/Kestrel/Core/test/HttpParserTests.cs

@@ -90,11 +90,15 @@ public void ParseRequestLineThrowsOnInvalidRequestLine(string requestLine)
            var buffer = new ReadOnlySequence<byte>(Encoding.ASCII.GetBytes(requestLine));
            var requestHandler = new RequestHandler();

+#pragma warning disable CS0618 // Type or member is obsolete
            var exception = Assert.Throws<BadHttpRequestException>(() =>


This is a good example of where we want to keep the pragmas so we can test that we're throwing the old exception type.

- StatusCode now simply returns the base.StatusCode - Static API from IIS and Kestrel BadHttpRequestException moved to respective IISBadHttpRequestException and KestrelBadHttpRequestException types - Most usages of deprecated IIS and Kestrel BadHttpRequestException moved to base type - Overload added to Microsoft.AspNetCore.Http.BadHttpRequestException to accept inner exception

Tratcher

Much better.

src/Servers/IIS/IIS/src/Core/IISHttpContext.cs

Tratcher · 2020-04-02T16:15:35Z

src/Servers/Kestrel/Core/src/Internal/Http/Http1MessageBody.cs

@@ -52,7 +52,7 @@ protected override Task OnConsumeAsync()
                    }
                }
            }
-            catch (BadHttpRequestException ex)
+            catch (Microsoft.AspNetCore.Http.BadHttpRequestException ex)


Using statement?

I can use a using directive here but it must go inside the namespace again.

src/Servers/Kestrel/Core/src/Internal/Http/HttpParser.cs

Tratcher · 2020-04-02T16:18:13Z

src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.cs

-        private BadHttpRequestException GetInvalidRequestTargetException(Span<byte> target)
-            => BadHttpRequestException.GetException(
+#pragma warning disable CS0618 // Type or member is obsolete
+        private Microsoft.AspNetCore.Server.Kestrel.Core.BadHttpRequestException GetInvalidRequestTargetException(Span<byte> target)


Base type/using?

Again callers of this method throw the returned exception so I think it needs to be the old obsoleted version?

Tratcher · 2020-04-03T17:16:20Z

Looks like a test issue in IIS.Tests.MaxRequestBodySizeTests.EveryReadFailsWhenContentLengthHeaderExceedsGlobalLimit:
https://dev.azure.com/dnceng/public/_build/results?buildId=587168&view=ms.vss-test-web.build-test-results-tab&runId=18503624&resultId=119834&paneView=debug

Assert.Equal() Failure

↓ (pos 9)

Expected: HTTP/1.1 413 Payload Too Large

Actual: HTTP/1.1 500 Internal Server E

↑ (pos 9)

I think it's this part:
https://github.com/dotnet/aspnetcore/pull/20339/files#diff-f7fe79bd73501e55cf962d965d1dce28R320-R323

                    requestRejectedEx1 = await Assert.ThrowsAsync<BadHttpRequestException>(
                        async () => await ctx.Request.Body.ReadAsync(buffer, 0, 1));
                    requestRejectedEx2 = await Assert.ThrowsAsync<BadHttpRequestException>(
                        async () => await ctx.Request.Body.ReadAsync(buffer, 0, 1));

ThrowsAsync checks for the exact exception type, not derived types. You can use ThrowsAnyAsync or check for the derived type here.

cado1982 · 2020-04-06T10:20:17Z

I applied the change you mentioned but I'm struggling to run the IIS tests locally. They run continuously and don't complete (for at least 2 hours). Was that the only one that was broken?

Tratcher · 2020-04-06T20:06:32Z

Looks like you fixed it. The remaining issue with Node seems to be unrelated, I'll see to that.

halter73 · 2020-04-06T20:31:49Z

src/Servers/Kestrel/Core/test/Http1ConnectionTests.cs

            var exception = Assert.Throws<BadHttpRequestException>(() => _http1Connection.TakeMessageHeaders(readableBuffer, trailers: false, out _consumed, out _examined));
+#pragma warning restore CS0618 // Type or member is obsolete


Nit: we could get rid of a lot of these pragmas in the tests by using Assert.ThrowsAny.

halter73 · 2020-04-06T20:33:22Z

src/Servers/Kestrel/Core/src/Internal/Http/Http1MessageBody.cs

@@ -87,7 +89,7 @@ protected async Task OnConsumeAsyncAwaited()
                    AdvanceTo(result.Buffer.End);
                } while (!result.IsCompleted);
            }
-            catch (BadHttpRequestException ex)
+            catch (Microsoft.AspNetCore.Http.BadHttpRequestException ex)


Nit: There's already a using that take's care of this, right?

I asked them to keep many of the test pragmas to make sure the right old exception type was being thrown.

halter73

Some of the test #pragma's could be removed, but nbd.

Tratcher · 2020-04-06T21:42:35Z

Thanks @cado1982

cado1982 · 2020-04-06T22:15:05Z

@halter73 @Tratcher Thanks. I learnt a lot from this.

cado1982 added 3 commits March 31, 2020 00:34

Add new BadHttpRequestException to Http Abstractions

7324562

Update Kestrel to use new BadHttpRequestException from Http Abstractions

a3e4b2c

Update IIS to use new BadHttpRequestException from Http Abstractions

0cd3e6f

cado1982 requested review from analogrelay, halter73, jkotalik and Tratcher as code owners March 30, 2020 23:45

ghost added the area-servers label Mar 30, 2020

cado1982 added 2 commits March 31, 2020 13:09

Add missing using directive to qualify StatusCodes

701d799

Regenerate reference assemblies

9df44c5

analogrelay modified the milestones: 5.0.0-preview3, 5.0.0-preview4 Mar 31, 2020

halter73 reviewed Mar 31, 2020

View reviewed changes

src/Servers/Kestrel/Core/src/BadHttpRequestException.cs Show resolved Hide resolved

halter73 reviewed Mar 31, 2020

View reviewed changes

src/Servers/Kestrel/Core/src/KestrelBadHttpRequestException.cs Outdated Show resolved Hide resolved

cado1982 added 4 commits April 1, 2020 10:35

Revert c7a356 f81b48 7b5611 aaea8b

1f60a99

Fix merge conflicts

68025ba

Alter BadHttpRequestException to inherit from shared Http Abstraction…

5247c0a

…s version - Mark existing BadHttpRequestException in IIS and Kestrel Obsolete - Add pragmas to ignore internal Obsolete usage

Regenerate reference assemblies

a905b04

Tratcher reviewed Apr 1, 2020

View reviewed changes

cado1982 added 2 commits April 2, 2020 11:24

Regenerate reference assemblies

9ae2034

Tratcher approved these changes Apr 2, 2020

View reviewed changes

Fix some usages of BadHttpRequestException and add using statements

32cf37c

cado1982 added 2 commits April 6, 2020 09:20

Merge branch 'master' into temp-11208

cfd0cef

Change test from ThrowsAny to ThrowsAnyAsync

1d2d166

Tratcher self-assigned this Apr 6, 2020

halter73 reviewed Apr 6, 2020

View reviewed changes

halter73 approved these changes Apr 6, 2020

View reviewed changes

Tratcher merged commit b463e04 into dotnet:master Apr 6, 2020

cado1982 deleted the temp-11208 branch April 6, 2020 22:15

Tratcher mentioned this pull request Apr 7, 2020

Announcement: BadHttpRequestException's obsolete and replaced #20614

Closed

amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Jun 6, 2023

		var exception = Assert.Throws<BadHttpRequestException>(() => _http1Connection.TakeMessageHeaders(readableBuffer, trailers: false, out _consumed, out _examined));
		#pragma warning restore CS0618 // Type or member is obsolete

Move BadHttpRequestException to Http.Abstractions #20339

Move BadHttpRequestException to Http.Abstractions #20339

Uh oh!

Conversation

cado1982 commented Mar 30, 2020

Uh oh!

dnfclas commented Mar 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Tratcher commented Mar 31, 2020

Uh oh!

Uh oh!

Uh oh!

analogrelay commented Mar 31, 2020

Uh oh!

cado1982 commented Mar 31, 2020

Uh oh!

halter73 commented Mar 31, 2020

Uh oh!

cado1982 commented Apr 1, 2020

Uh oh!

Tratcher commented Apr 1, 2020

Uh oh!

Tratcher left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Tratcher left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Tratcher commented Apr 3, 2020

Uh oh!

cado1982 commented Apr 6, 2020

Uh oh!

Tratcher commented Apr 6, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

halter73 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Tratcher commented Apr 6, 2020

Uh oh!

cado1982 commented Apr 6, 2020

Uh oh!

Uh oh!

dnfclas commented Mar 30, 2020 •

edited

Loading

halter73 left a comment •

edited

Loading