Use pipelines for SocketOutput #1497

davidfowl · 2017-03-17T11:54:31Z

Changed socket output to be based on pipelines
Changed connection filter glue to be based on pipelines
Codegen that used MemoryPoolIterator for output now uses WritableBuffer
Made UvWriteReq async/await friendly with LibuvAwaitable<T>
Deleted MemoryPool and friends

TODO:

~~Cancellation isn't handled, it needs to be.~~ Will do this in another PR once we get Add cancellation token support to FlushAsync and ReadAsync dotnet/corefxlab#1332
~~Little bit cleanup~~
~~Port relevant SocketOutput tests (they are commented out now)~~
Initial performance test

- Removed shutdown bools

- Use LibuvAwaitable instead of having all of that state on the UvWriteReq - Remove Console output from TestApplicationErrorLogger

- Reduce the lock in output to avoid deadlocks - Pin memory instead of using TryGetPointer in UvWriteReq

davidfowl · 2017-03-17T11:54:47Z

samples/SampleApp/Startup.cs


-                        listenOptions.UseConnectionLogging();
+                        // listenOptions.UseConnectionLogging();


Will revert

davidfowl · 2017-03-17T11:55:46Z

src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/ListenerContext.cs

+            MaximumSizeHigh = ServiceContext.ServerOptions.Limits.MaxResponseBufferSize ?? 0,
+            MaximumSizeLow = ServiceContext.ServerOptions.Limits.MaxResponseBufferSize ?? 0
+        };
+
        public PipeOptions AdaptedPipeOptions => new PipeOptions


This needs some tweaking. We don't want request limits for both pipes in the adapter.

davidfowl · 2017-03-17T11:56:06Z

src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/PipelineExtensions.cs

@@ -86,5 +89,19 @@ public static ArraySegment<byte> GetArray(this Memory<byte> memory)
            }
            return result;
        }
+
+        public static void WriteAscii(this WritableBuffer buffer, string data)


This needs to be optimized /cc @benaadams 😄

davidfowl · 2017-03-17T11:56:19Z

src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/PipelineExtensions.cs

+
+        public static void WriteNumeric(this WritableBuffer buffer, ulong number)
+        {
+            buffer.Write(number.ToString());


pakrym · 2017-03-17T14:16:54Z

src/Microsoft.AspNetCore.Server.Kestrel/Internal/Networking/UvWriteReq.cs

-                    Debug.Assert(tryGetPointerResult);
+                    // REVIEW: This isn't necessary for our default pool since the memory is 
+                    // already pinned but it also makes tests pass
+                    var memoryHandle = memory.Pin();


That test is wrong, passing ReadableBuffer.Create result which is array based and unexpected. We should just change the test.

It isn't wrong, just unnecessary if we were to use pre pinned buffers. I also think the overhead is negligible

benaadams · 2017-03-17T17:44:57Z

test/shared/TestApplicationErrorLogger.cs

@@ -17,7 +16,7 @@ public class TestApplicationErrorLogger : ILogger

        public bool ThrowOnCriticalErrors { get; set; } = true;

-        public List<LogMessage> Messages { get; } = new List<LogMessage>();
+        public ConcurrentBag<LogMessage> Messages { get; } = new ConcurrentBag<LogMessage>();


Going for eventually consistent model 😆

ConcurrentQueue?

- Deleted MemoryPool and friends and tests

cesarblum · 2017-03-17T18:37:39Z

src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/Connection.cs

                        adapterContext.ConnectionStream,
                        Thread.PipelineFactory.Create(ListenerContext.AdaptedPipeOptions),
-                        Thread.Memory,
-                        Log);
+                        Thread.PipelineFactory.Create(ListenerContext.AdaptedPipeOptions));


Use named args here.

Can't tell what is what.

We only do that for bools and null args.

cesarblum · 2017-03-17T18:40:03Z

src/Microsoft.AspNetCore.Server.Kestrel/Internal/Infrastructure/LibuvAwaitable.cs

+{
+    public class LibuvAwaitable<TRequest> : ICriticalNotifyCompletion where TRequest : UvRequest
+    {
+        private readonly static Action CALLBACK_RAN = () => { };


cesarblum · 2017-03-17T18:42:35Z

test/Microsoft.AspNetCore.Server.KestrelTests/SocketOutputTests.cs

+//namespace Microsoft.AspNetCore.Server.KestrelTests
+//{
+//    public class SocketOutputTests


So it works?

cesarblum · 2017-03-17T18:44:35Z

src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/ListenerContext.cs

+            ReaderScheduler = Thread,
+            WriterScheduler = ServiceContext.ThreadPool,
+            MaximumSizeHigh = ServiceContext.ServerOptions.Limits.MaxResponseBufferSize ?? 0,
+            MaximumSizeLow = ServiceContext.ServerOptions.Limits.MaxResponseBufferSize ?? 0


This looks like a new option is needed.

cesarblum · 2017-03-17T18:48:45Z

src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/SocketOutput.cs

-                        Self._head = null;
-                        Self._tail = null;
-                        returnAll = true;
+                        // REVIEW: Locking here


cesarblum · 2017-03-17T18:50:12Z

src/Microsoft.AspNetCore.Server.Kestrel/Internal/Infrastructure/LibuvAwaitable.cs

+
+namespace Microsoft.AspNetCore.Server.Kestrel.Internal.Infrastructure
+{
+    public class LibuvAwaitable<TRequest> : ICriticalNotifyCompletion where TRequest : UvRequest


nit: put where clause in it's own line

cesarblum · 2017-03-17T18:51:40Z

src/Microsoft.AspNetCore.Server.Kestrel/Internal/Infrastructure/LibuvAwaitable.cs

+
+    public struct UvWriteResult
+    {
+        public int Status;


cesarblum · 2017-03-17T18:52:38Z

src/Microsoft.AspNetCore.Server.Kestrel/Internal/Infrastructure/LibuvAwaitable.cs

+
+        private int _status;
+
+        public static Action<TRequest, int, Exception, object> Callback = (req, status, error, state) =>


pakrym · 2017-03-17T18:59:44Z

src/Microsoft.AspNetCore.Server.Kestrel/Adapter/Internal/AdaptedPipeline.cs

-            Input = pipe;
-            Output = new StreamSocketOutput(connectionId, filteredStream, memory, logger);
+            Input = inputPipe;
+            _output = new StreamSocketOutput(filteredStream, outputPipe);


Merge StreamSocketOutput into AdaptedPipeline

In the next round

pakrym · 2017-03-17T19:00:34Z

src/Microsoft.AspNetCore.Server.Kestrel/Adapter/Internal/StreamSocketOutput.cs

+                        if (buffer.IsEmpty)
+                        {
+                            await _outputStream.FlushAsync();
+                        }


pakrym · 2017-03-17T19:03:31Z

src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/SocketOutput.cs

        {
-            TaskCompletionSource<object> tcs = null;
-            var scheduleWrite = false;
+            var flushAwaiter = default(WritableBufferAwaitable);


WritableBufferAwaitable flushAwaiter;

pakrym · 2017-03-17T19:06:36Z

src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/SocketOutput.cs

-                        socketShutdownSend: false,
-                        socketDisconnect: true,
-                        isSync: true);
+                    // Not graceful


turn switch to if?

davidfowl · 2017-03-18T07:27:06Z

Will make all of the nit feedback in a subsequent commit.

davidfowl · 2017-03-18T07:32:46Z

This is merged

cesarblum · 2017-03-20T02:40:26Z

07cbf7f

pakrym and others added 15 commits March 17, 2017 04:42

Sample app works

a3e5a66

Tests build

b8b01d6

Conneciton filters work

a79fc51

How this even works?

91b65fe

Made some progress

3a2b249

- Removed shutdown bools

Change the timeout back to a minute

53b516b

Fix races

e88bcdf

More progress

e7413e3

Fix chunking in for connection adapters

6e31627

More fixes

d7424bd

- Use LibuvAwaitable instead of having all of that state on the UvWriteReq - Remove Console output from TestApplicationErrorLogger

Oops

97b1c02

More fixes

9bbc469

- Reduce the lock in output to avoid deadlocks - Pin memory instead of using TryGetPointer in UvWriteReq

Tests seem to be working

aae1b21

Small tweak to MockSocketOutput

6a90f54

Tests pass!

ef8b1a9

davidfowl requested review from halter73, cesarblum and pakrym March 17, 2017 11:54

dnfclas added the cla-already-signed label Mar 17, 2017

davidfowl commented Mar 17, 2017

View reviewed changes

Go back to concurrent bag for test logger

4969f0c

pakrym reviewed Mar 17, 2017

View reviewed changes

benaadams reviewed Mar 17, 2017

View reviewed changes

davidfowl added 3 commits March 17, 2017 10:45

Revert the sample changes

86a64f4

Less changes from the original

4fc5ac3

Remove dead code

4de0210

- Deleted MemoryPool and friends and tests

cesarblum reviewed Mar 17, 2017

View reviewed changes

pakrym reviewed Mar 17, 2017

View reviewed changes

davidfowl added 3 commits March 17, 2017 22:57

Bring back some socket output tests

4dfef63

Finish porting SocketOutput tests

8644a45

Pin CoreFxLab to avoid build breaks for now

e784618

davidfowl closed this Mar 18, 2017

davidfowl deleted the pakrym/pipe-output branch March 21, 2017 07:01

halter73 mentioned this pull request Mar 24, 2017

Graceful shutdown regression #1547

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use pipelines for SocketOutput #1497

Use pipelines for SocketOutput #1497

davidfowl commented Mar 17, 2017 •

edited

Loading

davidfowl Mar 17, 2017

davidfowl Mar 17, 2017

davidfowl Mar 17, 2017

davidfowl Mar 17, 2017

benaadams Mar 17, 2017

pakrym Mar 17, 2017

davidfowl Mar 17, 2017

benaadams Mar 17, 2017

cesarblum Mar 17, 2017

davidfowl Mar 17, 2017

cesarblum Mar 17, 2017

davidfowl Mar 18, 2017

cesarblum Mar 17, 2017

cesarblum Mar 17, 2017

cesarblum Mar 17, 2017

cesarblum Mar 17, 2017

cesarblum Mar 17, 2017

cesarblum Mar 17, 2017

cesarblum Mar 17, 2017

pakrym Mar 17, 2017

davidfowl Mar 18, 2017

pakrym Mar 17, 2017

pakrym Mar 17, 2017 •

edited

Loading

pakrym Mar 17, 2017

davidfowl commented Mar 18, 2017

davidfowl commented Mar 18, 2017

cesarblum commented Mar 20, 2017


		listenOptions.UseConnectionLogging();
		// listenOptions.UseConnectionLogging();


		private int _status;

		public static Action<TRequest, int, Exception, object> Callback = (req, status, error, state) =>

Use pipelines for SocketOutput #1497

Use pipelines for SocketOutput #1497

Conversation

davidfowl commented Mar 17, 2017 • 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

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pakrym Mar 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidfowl commented Mar 18, 2017

davidfowl commented Mar 18, 2017

cesarblum commented Mar 20, 2017

davidfowl commented Mar 17, 2017 •

edited

Loading

pakrym Mar 17, 2017 •

edited

Loading