Delay socket receive/send until first read/flush #34458

davidfowl · 2021-07-17T15:36:01Z

Today when the socket connection is accepted or connected, we implicitly start reading and writing from the socket. This can prevent certain scenarios where users want to get access to the raw socket before any operations happen (like in hand off scenarios). This change defers the reads and writes until read or flush is called on the transport's IDuplexPipe.

~~PS: I'd like to write a test that does the DuplicateAndClose but I need to figure out how involved that is (and how flaky the test would end up being...).~~
I added a test that does DuplicateAndClose to the same process to make sure the scenario works.

- Today when the socket connection is accepted or connection, we implicitly start reading and writing from the socket. This can prevent certain scenarios where users want to get access to the raw socket before any operations happen (like in hand off scenarios). This change defers the reads and writes until read or flush is called on the transport's IDuplexPipe.

…ket feature.

- Make sure we don't start multiple instances of reading and writing tasks because of thread safety issues.

gfoidl · 2021-07-19T10:58:38Z

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

        {
-            try
+            if (_connectionStarted == 1 || Interlocked.CompareExchange(ref _connectionStarted, 1, 0) == 1)


For the "fast-check" if (_connectionStarted == 1 should this be a volatile read?

Or from different view: EnsureStarted is only called in DisposeAsync so is it worth to have a fast-check? Just the compare & exchange should be enough?

No, it's called in more than 2 places. The fast check is in the hot path (every read/write/flush)

Ah, it's a partial class. Thanks.

But is still safe without a volatile read? Now I believe it's safe, as in the case of _connectionStarted = 0 the CompareExchange is hit which has the proper memory-fences.

kevin-montrose · 2021-07-19T15:59:41Z

src/Servers/Kestrel/test/Sockets.FunctionalTests/SocketTranspotTests.cs

 using Microsoft.AspNetCore.Server.Kestrel.FunctionalTests;
 using Microsoft.AspNetCore.Testing;
 using Microsoft.Extensions.Hosting;
 using Xunit;

+using KestrelHttpVersion = Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http.HttpVersion;
+using KestrelHttpMethod = Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http.HttpMethod;
+
 namespace Sockets.FunctionalTests
 {
    public class SocketTranspotTests : LoggedTestBase


Not introduced in this change, but file and type are misnamed. Should be SocketTransportTests .

mconnew · 2021-07-19T18:48:05Z

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

+
+            // Offload these to avoid potentially blocking the first read/write/flush
+            _receivingTask = Task.Run(DoReceive);
+            _sendingTask = Task.Run(DoSend);


Task.Run causes an extra Task allocation. The compiler generated Task returned by the DoReceive method, and the wrapping Task created by Task.Run. If you just place await Task.Yield().ConfigureAwait(false); as the first statement in DoReceive, you will avoid this extra allocation.

I can live with an extra allocation 😄. There's now 5 more here per connection. These are also long lived so I'm not concerned.

2 allocations because of DoSend, but fair enough.

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

halter73 · 2021-07-20T00:33:34Z

~~Any reason not to start send and receive loops independently?~~ Edit. The opposite is the better question. It's not worth it.

davidfowl · 2021-07-20T00:40:42Z

That's how it started but tests failed because they are dependent on both running. Specifically, the max request buffer size tests depend on the speed of the abort being received during the receive loop. So that can be fixed but I don't want to risk any subtle behavior changes here

halter73 · 2021-07-20T00:43:43Z

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

            Application = pair.Application;

+            Transport = new SocketDuplexPipe(this);


Nit:

Suggested change

Transport = new SocketDuplexPipe(this);

Transport = new SocketDuplexPipe(this, pair.Transport);

And remove InnerTransport. Edit: Or just reference _originalTransport directly in SocketDuplexPipe since it's nested.

I kind prefer using properties rather than a private field, even if it's internal. This is purely a stylistic thing though.

halter73 · 2021-07-20T00:51:01Z

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

@@ -106,6 +108,9 @@ public override void Abort(ConnectionAbortedException abortReason)
        // Only called after connection middleware is complete which means the ConnectionClosed token has fired.
        public override async ValueTask DisposeAsync()
        {
+            // Just in case we haven't started the connection, start it here so we can clean up properly.
+            EnsureStarted();


I guess we can remove the _receivingTask != null and _sendingTask != null checks now. Technically these could still be null if some background thread flipped _connectionStarted and hasn't set these yet, but that would indicate some misusage of the ConnectionContext during dispose.

Maybe it wouldn't be a misusage if the read or write was canceled before disposing.

davidfowl · 2021-07-20T04:32:46Z

I didn't run a performance profile, so we should look out for regressions here. EnsureStarted should be optimized but it's in every call to ReadAsync which could affect benchmarks...

@BrennanConroy @Pilchie @DamianEdwards @halter73 @sebastienros

halter73 · 2021-07-20T16:59:33Z

I'm more concerned with the extra struct copies than EnsureStarted, but I'm guessing both will be insignificant. Good to keep an eye on though.

davidfowl · 2021-07-20T17:00:28Z

I'm more concerned with the extra struct copies than EnsureStarted, but I'm guessing both will be insignificant. Good to keep an eye on though.

Struct copies?

davidfowl · 2021-07-20T17:00:57Z

You mean the wrapped PipeReader.ReadAsync() calls?

This reverts commit e2acbb9.

…35284) This reverts commit e2acbb9.

sebastienros · 2021-09-27T18:49:27Z

Might have had some impact on PlaintextMapAction

davidfowl · 2021-09-27T18:51:53Z

This change was reverted 952ccb4

sebastienros · 2021-09-27T18:52:55Z

Since it was reverted, next possibility is this PR:
#34285

davidfowl requested review from BrennanConroy, halter73 and Tratcher as code owners July 17, 2021 15:36

ghost added the area-runtime label Jul 17, 2021

davidfowl mentioned this pull request Jul 17, 2021

Add AcceptSocketAsync to SocketTransportOptions #34345

Closed

davidfowl added the feature-kestrel label Jul 17, 2021

davidfowl added 4 commits July 17, 2021 10:10

Added test to make sure we can read from the socket via IConnectionoc…

89d3582

…ket feature.

Make EnsureStarted thread safe

0109b8f

- Make sure we don't start multiple instances of reading and writing tasks because of thread safety issues.

Added DuplicateAndClose test

286af4a

Only run DuplicateAndClose test on windows

037aca6

gfoidl reviewed Jul 19, 2021

View reviewed changes

kevin-montrose reviewed Jul 19, 2021

View reviewed changes

mconnew reviewed Jul 19, 2021

View reviewed changes

halter73 reviewed Jul 20, 2021

View reviewed changes

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

halter73 reviewed Jul 20, 2021

View reviewed changes

halter73 approved these changes Jul 20, 2021

View reviewed changes

halter73 reviewed Jul 20, 2021

View reviewed changes

davidfowl merged commit e2acbb9 into main Jul 20, 2021

davidfowl deleted the davidfowl/lazy-socket-connection branch July 20, 2021 04:30

ghost added this to the 6.0-rc1 milestone Jul 20, 2021

halter73 mentioned this pull request Aug 11, 2021

Add ISocketConnectionContextFactory #34769

Merged

davidfowl added a commit that referenced this pull request Aug 12, 2021

Revert "Delay socket receive/send until first read/flush (#34458)"

5024bf6

This reverts commit e2acbb9.

davidfowl added a commit that referenced this pull request Aug 12, 2021

Revert "Delay socket receive/send until first read/flush (#34458)" (#…

952ccb4

…35284) This reverts commit e2acbb9.

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Delay socket receive/send until first read/flush #34458

Delay socket receive/send until first read/flush #34458

davidfowl commented Jul 17, 2021 •

edited

Loading

gfoidl Jul 19, 2021

davidfowl Jul 19, 2021

gfoidl Jul 19, 2021

kevin-montrose Jul 19, 2021

mconnew Jul 19, 2021

davidfowl Jul 19, 2021 •

edited

Loading

mconnew Jul 19, 2021

halter73 commented Jul 20, 2021 •

edited

Loading

davidfowl commented Jul 20, 2021

halter73 Jul 20, 2021 •

edited

Loading

davidfowl Jul 20, 2021

halter73 Jul 20, 2021

halter73 Jul 20, 2021

davidfowl commented Jul 20, 2021

halter73 commented Jul 20, 2021

davidfowl commented Jul 20, 2021

davidfowl commented Jul 20, 2021

sebastienros commented Sep 27, 2021

davidfowl commented Sep 27, 2021

sebastienros commented Sep 27, 2021

		Application = pair.Application;

		Transport = new SocketDuplexPipe(this);

	Transport = new SocketDuplexPipe(this);
	Transport = new SocketDuplexPipe(this, pair.Transport);

Delay socket receive/send until first read/flush #34458

Delay socket receive/send until first read/flush #34458

Conversation

davidfowl commented Jul 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidfowl Jul 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

halter73 commented Jul 20, 2021 • edited Loading

davidfowl commented Jul 20, 2021

halter73 Jul 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidfowl commented Jul 20, 2021

halter73 commented Jul 20, 2021

davidfowl commented Jul 20, 2021

davidfowl commented Jul 20, 2021

sebastienros commented Sep 27, 2021

davidfowl commented Sep 27, 2021

sebastienros commented Sep 27, 2021

davidfowl commented Jul 17, 2021 •

edited

Loading

davidfowl Jul 19, 2021 •

edited

Loading

halter73 commented Jul 20, 2021 •

edited

Loading

halter73 Jul 20, 2021 •

edited

Loading