Remove SocketException usage #38094

sebastienros · 2021-11-04T21:29:21Z

For review only. Not sure which benchmark would be impacted. It appears clearly in connection close, but there are other bottlenecks in this benchmark that make the change insignificant.

src/Servers/Kestrel/Transport.Sockets/src/Internal/TransferResult.cs

davidfowl · 2021-11-05T04:19:41Z

This needs to be opt in with an API if we do it. Then we can debate if it's on or off by default.

src/Servers/Kestrel/Transport.Sockets/src/Internal/TransferResult.cs

davidfowl · 2021-11-05T04:22:27Z

src/Servers/Kestrel/Transport.Sockets/src/Internal/TransferResult.cs

+
+namespace Microsoft.AspNetCore.Server.Kestrel.Transport.Sockets.Internal
+{
+    internal readonly struct TransferResult


Maybe this should just be a ValueTuple

As a base class or as-is? I wouldn't like to see the ValueTuple<> name everywhere it's used, not readable.

Does "Maybe" mean "please change" ?

halter73 · 2021-11-05T21:29:04Z

src/Servers/Kestrel/Transport.Sockets/src/Internal/SocketConnection.cs

+                        if (IsConnectionAbortError(result.SocketError!.SocketErrorCode))
+                        {
+                            // This exception should always be ignored because _shutdownReason should be set.
+                            error = result.SocketError!; 


Is this effectively treating socket errors that aren't considered either aborts or resets as "normal" completions? We definitely should throw for any socket error that we don't recognize.

We definitely should throw for any socket error that we don't recognize

It's still throwing the same exceptions outside of this component. This PR only removes any exceptions that were caught (expected) and not re-thrown.

halter73 · 2021-11-05T21:29:18Z

If we wanted to go further we don't even need to complete the Input pipe with an error. We could create a new transport feature that exposes the error that can be checked after "normal" completion. We'd only need to throw when app code is in the middle of reading a request body. Otherwise, we can handle the error without ever throwing or catching an exception.

I talked to @davidfowl about this a while back, but he didn't like the idea of a non-throwing transport because it would be hard to use. Maybe this would be acceptable if the feature gave a way to opt in to this behavior though.

src/Servers/Kestrel/Transport.Sockets/src/Internal/SocketOperationResult.cs

sebastienros · 2022-02-10T20:10:59Z

6 runs, take only 2 fastest.

application	base	pr	delta
CPU Usage (%)	31	28	-9.68%
Cores usage (%)	371	346	-6.74%
Working Set (MB)	137	133	-2.92%
Private Memory (MB)	305	156	-48.85%
Max CPU Usage (%)	31	28	-9.68%
Max Working Set (MB)	142	139	-2.11%
Max GC Heap Size (MB)	83	75	-9.64%
Size of committed memory by the GC (MB)	98	95	-3.06%
Max Number of Gen 0 GCs / sec	4	4	0.00%
Max Number of Gen 1 GCs / sec	0.5	0.5	0.00%
Max Number of Gen 2 GCs / sec	0	0	0.00%
Max Time in GC (%)	0	0	0.00%
Max Gen 0 Size (B)	2,393,444	3,490,112	45.82%
Max Gen 1 Size (B)	3,045,180	1,580,932	-48.08%
Max Gen 2 Size (B)	839,384	805,520	-4.03%
Max LOH Size (B)	99,024	99,024	0.00%
Max Allocation Rate (B/sec)	278,463,664	280,213,916	0.63%
Max GC Heap Fragmentation	27	42	55.56%
# of Assemblies Loaded	86	86	0.00%
Max Exceptions (#/s)	26,964	9	-99.97%
Max Lock Contention (#/s)	25	29	16.00%
Max ThreadPool Threads Count	24	29	20.83%
Max ThreadPool Queue Length	1	1	0.00%
Max ThreadPool Items (#/s)	161,866	168,894	4.34%
Max Active Timers	0	0	0.00%
IL Jitted (B)	176,076	172,964	-1.77%
Methods Jitted	1,980	1,919	-3.08%

load	base	pr	delta
CPU Usage (%)	32	23	-28.13%
Cores usage (%)	385	279	-27.53%
Working Set (MB)	38	38	0.00%
Private Memory (MB)	358	358	0.00%
Start Time (ms)	0	0	0.00%
First Request (ms)	58	75	29.31%
Requests/sec	26,541	28,078	5.79%
Requests	214,754	227,090	5.74%
Mean latency (ms)	6.16	5.93	-3.73%
Max latency (ms)	41.68	35.88	-13.92%
Bad responses	0	0	0.00%
Socket errors	0	0	0.00%
Read throughput (MB/s)	3.59	3.8	5.85%
Latency 50th (ms)	6.12	5.87	-4.08%
Latency 75th (ms)	6.26	6.16	-1.60%
Latency 90th (ms)	6.85	6.48	-5.40%
Latency 99th (ms)	8.65	7.55	-12.72%

BrennanConroy · 2022-02-10T21:01:54Z

Seems like mostly good numbers. The heap fragmentation one is a bit odd

sebastienros · 2022-02-10T21:04:58Z

This test failed twice: Microsoft.AspNetCore.Server.IIS.FunctionalTests.Http2TrailerResetTests.Reset_DuringRequestBody_Resets

I don't remember it was failing before I merged main. Maybe I made a mistake, will check.

davidfowl · 2022-02-10T23:21:56Z

Did we address the breaking change somehow?

sebastienros · 2022-02-11T16:48:29Z

Did we address the breaking change somehow?

I don't believe there is any breaking change. Please let me know which one I am missing.

davidfowl · 2022-02-25T02:50:08Z

@sebastienros #38094 (comment)

halter73 · 2022-02-26T00:34:10Z

This still completes the pipes with exceptions, so we don't need to make a breaking change. My suggestion was to not even throw from the Pipe read by the application. That'd probably be an improvement, but I don't think that's necessary. This still removes half of the throwing and catching in many cases.

halter73 · 2022-02-26T00:36:51Z

src/Servers/Kestrel/Transport.Sockets/src/Internal/SocketConnection.cs

+                        return false;
+                    }
+
+                    return true;


This should never return true without setting error to some non-null value.

I misread this initially. If result.HasError, we should always be returning false. And this should never return false without setting error to some non-null value.

We are now ignoring "unknown" errors. There are no tests for this, because these errors are unknown and we don't know how to reproduce it, but we definitely should not be swallowing these.

Assuming result.BytesTransferred is 0 given an unkown error (which seems like a safe bet), then the error will get logged as a FIN which is terrible for debugging. If result.BytesTransferred is somehow non-zero, it'd be even worse.

Suggested change

return true;

// This is unexpected.

error = result.SocketError;

SocketsLog.ConnectionError(_logger, this, error);

return false;

halter73 · 2022-02-26T00:38:42Z

src/Servers/Kestrel/Transport.Sockets/src/Internal/SocketConnection.cs

+
+                        break;
+                    }
+


If we've gotten to this point and transferResult.HasError, we should be doing unexpectedError = shutdownReason = transferResult.SocketError.

I did the change, but I don't think it was the behavior before.

davidfowl · 2022-04-16T02:32:04Z

Sounds like a great improvement. Are we taking this?

sebastienros · 2022-04-19T17:06:22Z

All is green, any takers to approve it?

adityamandaleeka

Looks good. The one thing I'd add is a comment somewhere (maybe in SocketOperationResult) that explains why this is being done. Otherwise, someone could come along later and undo this 😄.

sebastienros added 3 commits November 3, 2021 18:38

Reduce dependency on exceptions

766f4ef

Cleanup

97bf2c3

Fix return value

5ddc912

sebastienros requested review from BrennanConroy, Tratcher and halter73 as code owners November 4, 2021 21:29

ghost added the area-runtime label Nov 4, 2021

BrennanConroy reviewed Nov 4, 2021

View reviewed changes

src/Servers/Kestrel/Transport.Sockets/src/Internal/TransferResult.cs Outdated Show resolved Hide resolved

davidfowl self-requested a review November 5, 2021 04:19

davidfowl reviewed Nov 5, 2021

View reviewed changes

src/Servers/Kestrel/Transport.Sockets/src/Internal/TransferResult.cs Outdated Show resolved Hide resolved

davidfowl reviewed Nov 5, 2021

View reviewed changes

src/Servers/Kestrel/Transport.Sockets/src/Internal/TransferResult.cs Outdated Show resolved Hide resolved

davidfowl reviewed Nov 5, 2021

View reviewed changes

Rename to SocketOperationResult

45d9ff5

halter73 reviewed Nov 5, 2021

View reviewed changes

BrennanConroy reviewed Nov 8, 2021

View reviewed changes

src/Servers/Kestrel/Transport.Sockets/src/Internal/SocketOperationResult.cs Outdated Show resolved Hide resolved

sebastienros added 4 commits February 10, 2022 08:38

Use file namespace

9584a60

Format documents

e107bfc

Merge branch 'main' into sebros/ex

f52a3e2

Fix type

44b81ae

adityamandaleeka assigned halter73 and davidfowl Feb 25, 2022

halter73 reviewed Feb 26, 2022

View reviewed changes

Tratcher mentioned this pull request Apr 15, 2022

Kestrel uses exception for control flow when handling the "Connection": "close" header. #41206

Closed

1 task

PR feedback

07f9c58

sebastienros requested a review from JamesNK as a code owner April 19, 2022 15:27

adityamandaleeka approved these changes Apr 29, 2022

View reviewed changes

adityamandaleeka merged commit 6a7bdc0 into main May 4, 2022

adityamandaleeka deleted the sebros/ex branch May 4, 2022 22:44

ghost added this to the 7.0-preview5 milestone May 4, 2022

davidfowl added the Perf label Aug 26, 2022

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

amcasey mentioned this pull request Nov 9, 2023

System.Net.Sockets.SocketException (995) #51788

Open

github-actions bot locked and limited conversation to collaborators Dec 8, 2023

-                    return true;
+                    // This is unexpected.
+                    error = result.SocketError;
+                    SocketsLog.ConnectionError(_logger, this, error);
+                    return false;

Remove SocketException usage #38094

Remove SocketException usage #38094

Uh oh!

Conversation

sebastienros commented Nov 4, 2021

Uh oh!

Uh oh!

davidfowl commented Nov 5, 2021

Uh oh!

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

halter73 commented Nov 5, 2021

Uh oh!

Uh oh!

sebastienros commented Feb 10, 2022

Uh oh!

BrennanConroy commented Feb 10, 2022

Uh oh!

sebastienros commented Feb 10, 2022

Uh oh!

davidfowl commented Feb 10, 2022

Uh oh!

sebastienros commented Feb 11, 2022

Uh oh!

davidfowl commented Feb 25, 2022

Uh oh!

halter73 commented Feb 26, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

halter73 May 9, 2022 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidfowl commented Apr 16, 2022

Uh oh!

sebastienros commented Apr 19, 2022

Uh oh!

adityamandaleeka left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

halter73 May 9, 2022 •

edited

Loading