[7.0-rc2] Allow spaces between the url and http version #43704

[Tratcher]

Allow spaces between the url and http version

Description

This is a change to allow requests that have extra spaces between the url and http version. E.g. "GET(sp)/(sp)(sp)HTTP/1.1\r\n". The spec only allows one space, but some clients send more than one and IIS/Http.Sys allow it.

Contributes to #43705

Customer Impact

The customer regressed when moved to Kestrel. The customer doesn't expect to be able to update/replace the affected clients for 3+ years.

Regression?

• Yes
• No
• Umm...

Not a regression in Kestrel, but a compat break for customers moving from IIS/Http.Sys.

Risk

• High
• Medium
• Low

Constrained, unit testable.

Verification

• Manual (required)
• Automated

Packaging changes reviewed?

• Yes
• No
• N/A

[ghost]

Hi @Tratcher. Please make sure you've updated the PR description to use the Shiproom Template. Also, make sure this PR is not marked as a draft and is ready-to-merge.

To learn more about how to prepare a servicing PR click here.

[JamesNK]

I guess there will be a small perf hit in HTTP/1.1 parsing from this extra check. Check benchmarks?

I wonder whether it would be faster to integrate logic to skip extra spaces to the for loops in path and query string that check for a space.

[Tratcher]

/benchmark plaintext aspnet-perf-lin kestrel

[pr-benchmarks]

Benchmark started for plaintext on aspnet-perf-lin with kestrel

[pr-benchmarks]

plaintext - aspnet-perf-lin

application	plaintext.base	plaintext.pr
CPU Usage (%)	84	87	+3.57%
Cores usage (%)	1,002	1,048	+4.59%
Working Set (MB)	191	190	-0.52%
Private Memory (MB)	736	704	-4.35%
Build Time (ms)	5,646	2,814	-50.16%
Start Time (ms)	125	128	+2.40%
Published Size (KB)	98,073	98,073	0.00%
.NET Core SDK Version	8.0.100-alpha.1.22452.5	8.0.100-alpha.1.22452.5

load	plaintext.base	plaintext.pr
CPU Usage (%)	98	99	+1.02%
Cores usage (%)	1,175	1,190	+1.28%
Working Set (MB)	49	38	-22.45%
Private Memory (MB)	370	370	0.00%
Build Time (ms)	8,582	0
Start Time (ms)	0	0
Published Size (KB)	76,697	0
.NET Core SDK Version	3.1.422	0
First Request (ms)	60	60	0.00%
Requests/sec	5,356,342	5,243,627	-2.10%
Requests	80,841,520	79,168,320	-2.07%
Mean latency (ms)	5.24	5.75	+9.73%
Max latency (ms)	77.10	54.10	-29.83%
Bad responses	0	0
Socket errors	0	0
Read throughput (MB/s)	643.63	630.09	-2.10%
Latency 50th (ms)	2.96	3.33	+12.50%
Latency 75th (ms)	7.78	8.77	+12.72%
Latency 90th (ms)	13.58	14.97	+10.24%
Latency 99th (ms)	24.41	25.79	+5.65%

[davidfowl]

@sebastienros can we change PR benchmarks to optionally compare the exact same number of requests?

[sebastienros]

@davidfowl such a niche case that maybe at that point better to trigger it from the command line tool and add the special crank arguments you want.

Otherwise I might add a special profile for that. I don't recall if they are already handles or if something needs to be implemented. Wouldn't hurt if not. And provide a list of available extra profiles to use with their descriptions.

[Pilchie]

Approved for .NET 7 RC2.

[Tratcher]

Reviewers, please approve. If we want to attempt further optimizations, we can do that in 8.

[halter73]

Out of curiosity, do we know if HTTP.sys would accept any of the above invalid requests?

[Tratcher]

@wtgodbe @adityamandaleeka please merge.

[sebastienros]

/benchmark plaintext,json aspnet-citrine-lin,aspnet-citrine-win kestrel