Treat ECONNRESET as a connection error (#934). #953

cesarblum · 2016-07-05T23:05:12Z

cesarblum · 2016-07-06T23:29:07Z

Rebased.

halter73 · 2016-07-07T00:11:57Z

test/Microsoft.AspNetCore.Server.Kestrel.FunctionalTests/RequestTests.cs

+                .UseUrls($"http://127.0.0.1:0")
+                .Configure(app => app.Run(context =>
+                {
+                    return Task.FromResult(0);


We should also assert that context.Request.Body.ReadAsync fails with an IOException.

I think that SocketInputExtensions.ReadAsync and possibly a few other places will have to be updated to check if there is an error (possibly by calling SocketInput.GetResult()) in a thread-safe way before exiting due to RemoteIntakeFin being set.

I've opened #959 to address this.

mikeharder · 2016-07-07T19:32:26Z

test/Microsoft.AspNetCore.Server.Kestrel.FunctionalTests/RequestTests.cs

+                var filteredWrites = testSink.Writes.Where(write => write.EventId.Id == connectionErrorEventId);
+                for (var i = 0; i < 10; i++)
+                {
+                    System.Console.WriteLine(i);


Remove call to Console.WriteLine()?

mikeharder · 2016-07-09T01:26:41Z

test/Microsoft.AspNetCore.Server.Kestrel.FunctionalTests/RequestTests.cs

+
+                // Give it some time for the connection error log message to be logged
+                var filteredWrites = testSink.Writes.Where(write => write.EventId.Id == connectionErrorEventId);
+                for (var i = 0; i < 10; i++)


I would make the test code itself wait forever (rather than only 500ms), then wrap the whole test in a call to Task.Run().TimeoutAfter() with a large timeout (say 10 seconds), so it fails if it exceeds the timeout for any reason.

cesarblum · 2016-07-11T22:53:42Z

@halter73 Now handling error when checking for FIN.

davidfowl · 2016-07-11T23:22:29Z

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

            }
        }
+
+        private void ThrowOnConnectionError()


@davidfowl You're now well-versed in this code. Do you have a better alternative?

Can we just use a Task instead?

I also like this idea @CesarBS.

cesarblum · 2016-07-13T01:35:52Z

Updated to use TaskCompletionSource to combine FIN and error states, as suggested by @davidfowl.

halter73 · 2016-07-13T04:47:56Z

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

        private bool _consuming;
        private bool _disposed;

+        private TaskCompletionSource<bool> _tcs = new TaskCompletionSource<bool>();


TaskCompletionSource<bool> implies Result is useful and might be false. Since it never will be, TaskCompletionSource<object> is probably better.

halter73 · 2016-07-14T00:17:14Z

cesarblum · 2016-07-14T22:13:30Z

Updated tests:

Check type of InnerException inside caught IOException (@halter73 we get a UvException here, not a TaskCanceledException)
Add timeouts to WaitAsync calls on semaphores
Use custom test sink to look for expected log message on connection reset. With the previous approach there would sometimes be errors when calling Any() on the writes collection while it was being modified

cesarblum · 2016-07-14T23:51:43Z

Had to change ThrowsOnReadAfterConnectionError to handle two possible exception types thrown on an attempt to read from an errored connection:

IOException: this was in the original test. We get one if ReadAsync is awaiting on SocketInput when the connection is aborted.
TaskCanceledException: this is the new one. We get one if ReadAsync is called by the time the connection has already been aborted after an error.

This is quite ugly. We should probably address this and have a single behavior, but I'd like it to be addressed as a separate task.

dnfclas added the cla-already-signed label Jul 5, 2016

cesarblum force-pushed the cesarbs/econnreset-as-error branch from 4b960e1 to c42ab12 Compare July 6, 2016 23:28

halter73 reviewed Jul 7, 2016
View reviewed changes

cesarblum mentioned this pull request Jul 7, 2016

Check for connection errors when attempting to read request data #959

Closed

mikeharder reviewed Jul 7, 2016
View reviewed changes

cesarblum force-pushed the cesarbs/econnreset-as-error branch from c42ab12 to 7b4a6ab Compare July 7, 2016 23:00

mikeharder reviewed Jul 9, 2016
View reviewed changes

cesarblum force-pushed the cesarbs/econnreset-as-error branch from 7b4a6ab to 122ea86 Compare July 11, 2016 22:52

davidfowl reviewed Jul 11, 2016
View reviewed changes

cesarblum force-pushed the cesarbs/econnreset-as-error branch from ccd701a to ff13219 Compare July 13, 2016 01:35

halter73 reviewed Jul 13, 2016
View reviewed changes

Treat ECONNRESET as a connection error (#934).

3bb7f4e

cesarblum force-pushed the cesarbs/econnreset-as-error branch from 27334f5 to 3bb7f4e Compare July 15, 2016 21:54

cesarblum merged commit 3bb7f4e into dev Jul 15, 2016

cesarblum deleted the cesarbs/econnreset-as-error branch July 15, 2016 22:07

natemcmaster mentioned this pull request Aug 25, 2016

Kestrel + IIS is falling over with a small load in production with RTM aspnet/IISIntegration#245

Closed

Treat ECONNRESET as a connection error (#934). #953

Treat ECONNRESET as a connection error (#934). #953

Uh oh!

Conversation

cesarblum commented Jul 5, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cesarblum commented Jul 6, 2016

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!

mikeharder Jul 9, 2016 • 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!

cesarblum commented Jul 11, 2016

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!

cesarblum commented Jul 13, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

halter73 commented Jul 14, 2016

Uh oh!

cesarblum commented Jul 14, 2016

Uh oh!

cesarblum commented Jul 14, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

cesarblum commented Jul 5, 2016 •

edited

Loading

mikeharder Jul 9, 2016 •

edited

Loading