Improve testing of Sockets transport #2100

halter73 · 2017-10-05T23:03:28Z

This PR also makes Sockets the default. This will allow us to see the quickly see the impact of selecting the Sockets transport in ServerTests/MusicStore/Entropy/etc.

#1694

davidfowl · 2017-10-05T23:41:55Z

src/Kestrel/WebHostBuilderKestrelExtensions.cs

            return hostBuilder.ConfigureServices(services =>
            {
+                // Don't override an already-configured transport
+                services.TryAddSingleton<ITransportFactory, SocketTransportFactory>();


Why not call UseSockets()

Calling UseSockets here would cause UseKestrel override any previously configured transport. This is different than the old behavior with the libuv default, but it makes the TransportSelectors easier to implement.

cesarblum · 2017-10-06T01:52:30Z

KestrelHttpServer.sln

+EndProject
+Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Kestrel.Trasnport.Libuv.FunctionalTests", "test\Kestrel.Transport.Libuv.FunctionalTests\Kestrel.Trasnport.Libuv.FunctionalTests.csproj", "{74032D79-8EA7-4483-BD82-C38370420FFF}"
+EndProject
+Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Kestrel.Trasnport.Sockets.FunctionalTests", "test\Kestrel.Transport.Sockets.FunctionalTests\Kestrel.Trasnport.Sockets.FunctionalTests.csproj", "{9C7B6B5F-088A-436E-834B-6373EA36DEEE}"


typo: Trasnport

cesarblum · 2017-10-06T01:53:57Z

src/Kestrel.Transport.Sockets/Internal/ISocketTrace.cs

+
+namespace Microsoft.AspNetCore.Server.Kestrel.Transport.Sockets.Internal
+{
+    public interface ISocketTrace : ILogger


I think this should be called ISocketsTrace, given the transport is called Sockets.

cesarblum · 2017-10-06T01:54:19Z

src/Kestrel.Transport.Sockets/Internal/SocketTrace.cs

+
+namespace Microsoft.AspNetCore.Server.Kestrel.Transport.Sockets.Internal
+{
+    public class SocketTrace : ISocketTrace


Isn't this exactly (or almost exactly) the same as LibuvTrace?

Yes. Minus a few things.

Then how about having a common base class in Transport.Abstractions? Do you think that might not suit future transports?

It could be a good idea, but it's not 100% clear exactly which log methods belong in the base class. Some transports might not be TCP-based and not have a concept of things like sending vs receiving a FIN.

I created #2101 to discuss this idea further.

cesarblum · 2017-10-06T01:55:02Z

src/Kestrel.Transport.Sockets/Internal/SocketTrace.cs

+        // ConnectionRead: Reserved: 3
+
+        private static readonly Action<ILogger, string, Exception> _connectionPause =
+            LoggerMessage.Define<string>(LogLevel.Debug, 4, @"Connection id ""{ConnectionId}"" paused.");


Won't these IDs clash with other IDs? What's the scope of an event ID?

The IDs match mostly to make the cross-transport tests easier. It's in the "Microsoft.AspNetCore.Server.Kestrel.Transport.Sockets" scope, so there shouldn't be any conflicts.

cesarblum · 2017-10-06T01:57:13Z

src/Kestrel/Kestrel.csproj

@@ -16,7 +16,9 @@

  <ItemGroup>
    <ProjectReference Include="..\Kestrel.Core\Kestrel.Core.csproj" />
-    <ProjectReference Include="..\Kestrel.Transport.Libuv\Kestrel.Transport.Libuv.csproj" />
+    <!-- Even though the Libuv transport is no longer used by default, it's remains for back-compat -->


cesarblum · 2017-10-06T02:02:21Z

KestrelHttpServer.sln

+		test\SystemdActivation\Dockerfile = test\SystemdActivation\Dockerfile
+	EndProjectSection
+EndProject
+Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Kestrel.Trasnport.Libuv.FunctionalTests", "test\Kestrel.Transport.Libuv.FunctionalTests\Kestrel.Trasnport.Libuv.FunctionalTests.csproj", "{74032D79-8EA7-4483-BD82-C38370420FFF}"


typo: Trasnport

davidfowl · 2017-10-06T02:13:06Z

src/Kestrel.Transport.Sockets/SocketConnection.cs

@@ -223,7 +244,13 @@ private void SetupSendBuffers(ReadableBuffer buffer)
            }
            finally
            {
+                // Now, complete the input so that no more reads can happen
+                Input.Complete(error ?? new ConnectionAbortedException());


Do we need to do this here? It happens on the input side. It's a bit cleaner than the libuv transport.

https://github.com/aspnet/KestrelHttpServer/blob/dev/test/Kestrel.FunctionalTests/RequestTests.cs#L1081-L1091

If we don't complete the input with a ConnectionAbortedException here prior to initiating a connection shutdown, the test will close the client half of the connection which will complete the input gracefully.

If something is still trying to read after the output is closed, that's a problem. The connection is aborted at this point. The exact same code is there for libuv: https://github.com/aspnet/KestrelHttpServer/blob/dev/src/Kestrel.Transport.Libuv/Internal/LibuvConnection.cs#L72-L82

The feature is fine I just want it to happen at the "right place" which IMO is the end of the input loop.

https://github.com/aspnet/KestrelHttpServer/blob/dev/src/Kestrel.Transport.Sockets/SocketConnection.cs#L208-L215

I think you linked to the end of the output loop. Just like the LibuvConnection, SocketConnection completes the input both when the output is closed and the input is closed. This means input is always completed twice in both transports.

I meant this:

https://github.com/aspnet/KestrelHttpServer/blob/dev/src/Kestrel.Transport.Sockets/SocketConnection.cs#L111-L122

I see. Well, it's removed now and we're back to one call to IPipeWriter.Complete(). I'm wondering if we should change the libuv transport as well.

It's easier here since there's 2 async loops

True. If the libuv output loop exits first, it will call uv_close which prevents the read callback from receiving notification of the disconnect.

- No RTM version of this package has been shipped.

- Logging improvements

- Mostly fixed typos

cesarblum · 2017-10-09T21:44:10Z

src/Kestrel.Transport.Sockets/SocketConnection.cs

-                // Make sure to close the connection only after the input is aborted
+                // Make sure to close the connection only after the _aborted flag is set.
+                // Without this, the RequestsCanBeAbortedMidRead test will sometimes fail when
+                // a BadHttpRequestException is thrown instaed of a TaskCanceledException.


nit: instead

davidfowl · 2017-10-10T02:40:30Z

Why is Travis failing?

davidfowl

When Travis passes ..

davidfowl · 2017-10-10T02:35:58Z

src/Kestrel.Transport.Sockets/SocketConnection.cs

@@ -40,6 +46,10 @@ internal SocketConnection(Socket socket, SocketTransport transport)
            RemotePort = remoteEndPoint.Port;
        }

+        public override PipeFactory PipeFactory { get; }
+        public override IScheduler InputWriterScheduler => InlineScheduler.Default;


Is this problematic?

davidfowl · 2017-10-10T05:02:39Z

Seems like there are a bunch of legit failures on OSX.

muratg · 2017-10-10T18:21:49Z

src/Kestrel.Transport.Sockets/SocketConnection.cs

            }
            finally
            {
+                if (_aborted)
+                {
+                    error = error ?? new ConnectionAbortedException();


Should we log in the ConnectionAbortedException case?

Just because the error is being set here, doesn't mean that there is an ongoing call to Request.Body.ReadAsync() for the error to be thrown from. This error always gets set for a server-initiated connection close. If there is an ongoing ReadAsync() call when the server closes the connection, it's because the application called Abort().

We log a disconnect in the Abort() call that sets _aborted flag here. This covers both the libuv and sockets transports.

We could look to changing the log in Http1OutputProducer to make it more clear whether the connection was aborted or closed normally, though this is usually clear when the exception from ReadAsync bubbles up and gets logged.

halter73 requested review from davidfowl and cesarblum October 5, 2017 23:03

dnfclas added the cla-already-signed label Oct 5, 2017

davidfowl reviewed Oct 5, 2017

View reviewed changes

cesarblum suggested changes Oct 6, 2017

View reviewed changes

davidfowl reviewed Oct 6, 2017

View reviewed changes

halter73 force-pushed the halter73/sockets branch from dcce7b7 to 549c394 Compare October 6, 2017 02:23

halter73 added 10 commits October 6, 2017 15:30

Make Sockets the default transport

f7304e1

Fix failing functional tests post Sockets transport default

05472f5

Moved OneToTenThreads test to Kestrel.Transport.Libuv.Tests

18b53b4

Create separate Libuv and Sockets functional test projects

67f937c

Update socket transport baseline

49a0be5

- No RTM version of this package has been shipped.

Kill Socket input as soon as the output is closed

272e450

- Logging improvements

Fix socket close ordering

0e6125a

Fix systemd activation tests to use libuv transport

92c829a

Address PR feedback

e47da90

- Mostly fixed typos

Fully close socket after server-side abort

7f88ba8

halter73 force-pushed the halter73/sockets branch from 549c394 to 7f88ba8 Compare October 6, 2017 23:34

halter73 added 2 commits October 6, 2017 17:41

Dispose Sockets PipeFactory

9707939

Improve Socket's server-side abort handling

b49fde0

cesarblum reviewed Oct 9, 2017

View reviewed changes

cesarblum approved these changes Oct 9, 2017

View reviewed changes

Fix typo

f157b4a

davidfowl approved these changes Oct 10, 2017

View reviewed changes

muratg reviewed Oct 10, 2017

View reviewed changes

Prevent port 5000 binding errors on macOS

ac6350b

halter73 force-pushed the halter73/sockets branch 2 times, most recently from 9e4ea06 to e918edd Compare October 11, 2017 19:56

halter73 force-pushed the halter73/sockets branch from e918edd to fded4e9 Compare October 11, 2017 20:55

Add explicit rebinding test

adfcc82

halter73 force-pushed the halter73/sockets branch from fded4e9 to adfcc82 Compare October 11, 2017 21:34

halter73 merged commit fdb4184 into dev Oct 11, 2017

halter73 deleted the halter73/sockets branch October 11, 2017 22:14

natemcmaster mentioned this pull request Mar 5, 2018

Switch default transport to sockets #2360

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve testing of Sockets transport #2100

Improve testing of Sockets transport #2100

halter73 commented Oct 5, 2017

davidfowl Oct 5, 2017

halter73 Oct 6, 2017

cesarblum Oct 6, 2017

cesarblum Oct 6, 2017

cesarblum Oct 6, 2017

halter73 Oct 6, 2017

cesarblum Oct 6, 2017

halter73 Oct 6, 2017

cesarblum Oct 6, 2017

halter73 Oct 6, 2017

cesarblum Oct 6, 2017

cesarblum Oct 6, 2017

davidfowl Oct 6, 2017

halter73 Oct 6, 2017

davidfowl Oct 6, 2017

halter73 Oct 6, 2017 •

edited

Loading

davidfowl Oct 7, 2017

halter73 Oct 7, 2017

davidfowl Oct 7, 2017

halter73 Oct 7, 2017

cesarblum Oct 9, 2017

davidfowl commented Oct 10, 2017 •

edited

Loading

davidfowl left a comment

davidfowl Oct 10, 2017

davidfowl commented Oct 10, 2017

muratg Oct 10, 2017

halter73 Oct 10, 2017

Improve testing of Sockets transport #2100

Improve testing of Sockets transport #2100

Conversation

halter73 commented Oct 5, 2017

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

halter73 Oct 6, 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

davidfowl commented Oct 10, 2017 • edited Loading

davidfowl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidfowl commented Oct 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

halter73 Oct 6, 2017 •

edited

Loading

davidfowl commented Oct 10, 2017 •

edited

Loading