[Needs Tests] Add timeouts #485

benaadams · 2015-12-14T14:49:25Z

Still needs

KeepAliveTimeout
HeadersCompleteTimeout
ExecutionTimeout
Read from config
Handle bad config
Switching off on Upgrades (e.g. WebSockets)
Cancelled/Aborted Exception (Easier to distinguish connection close events over more troubling errors)
Tests

muratg · 2015-12-14T15:21:01Z

src/Microsoft.AspNet.Server.Kestrel/Http/Frame.cs

+        }
+        protected int TransitionToState(int state)
+        {
+#pragma warning disable CS0420 // A reference to a volatile field will not be treated as volatile


Isn't disabling this potentially dangerous?

Is a spurious compile warning though perhaps I can just go
int prevState = _frameState;
rather than
int prevState = Volatile.Read(ref _frameState);
and not get the error

Yeah, my worry is that passing volatile fields as ref (or out) may have different behavior on different CPU architectures.

Now you raise is... changed to normal int and changed all reads to Volatile.Read as while volatile gives compiler guarantees which is enough on Intel; it doesn't give CPU guarantees which may nor be enough on arm. (Don't know how the traditional C# memory memory model works on other chipssets). Also I'm using the value with Interlocked which wants ref

Also I've rolled two flags into one state and added some extras; so safety first.

benaadams · 2015-12-15T03:45:20Z

Added keep-alive as: server.keepAliveTimeout also server.executionTimeout
However as kestrel.headersCompleteTimeout
That work?

cesarblum · 2016-01-11T18:48:32Z

src/Microsoft.AspNet.Server.Kestrel/Http/Frame.cs

+            {
+                SocketInput?.AbortAwaiting();
+            }
+        }


nit: add newline

benaadams · 2016-01-12T03:47:36Z

Just need to add tests (I think)

cesarblum · 2016-01-12T16:43:04Z

Yes, please 😄

benaadams · 2016-01-12T23:30:06Z

Check pointing. Still needs more work in light of #566 (comment)

benaadams · 2016-01-13T00:18:02Z

Should have it finished tomorrow.

halter73 · 2016-01-13T00:21:56Z

src/Microsoft.AspNet.Server.Kestrel/Http/SocketOutput.cs

@@ -388,6 +387,22 @@ void ISocketOutput.Write(ArraySegment<byte> buffer, bool immediate, bool chunk)

        Task ISocketOutput.WriteAsync(ArraySegment<byte> buffer, bool immediate, bool chunk, CancellationToken cancellationToken)
        {
+            if (cancellationToken.IsCancellationRequested)


We want to register with the cancellation token for writes that actually result in an incomplete task being returned.

Done; not sure perf effects; but on the plus side I've removed some boxing and unboxing 😜

benaadams · 2016-01-13T11:58:49Z

Checkpoint

benaadams · 2016-01-13T12:59:13Z

src/Microsoft.AspNet.Server.Kestrel/Http/SocketInput.cs

@@ -238,6 +239,14 @@ public void GetResult()
            var error = _awaitableError;
            if (error != null)
            {
+                if (error is OperationCanceledException)


Wrong error handling

benaadams · 2016-01-13T13:00:21Z

Question on how to handle error in ProducingStart

benaadams · 2016-02-02T23:20:47Z

build stalls when merged with the dotnet update - investigating

cesarblum · 2016-02-02T23:36:37Z

😞 Try setting NO_PARALLEL_TEST_PROJECTS=Microsoft.AspNetCore.Server.Kestrel.FunctionalTests (env. variable).

benaadams · 2016-02-02T23:56:58Z

Having a little look as it works without the merge; trying just upping threads e.g.
set ComPlus_ThreadPool_ForceMinWorkerThreads=2000

muratg · 2016-02-03T00:15:43Z

src/Microsoft.AspNetCore.Server.Kestrel/KestrelServerInformation.cs

+
+        private static TimeSpan GetTimeout(IConfiguration configuration, string configurationKey, int defaultSeconds)
+        {
+            var timeoutString = configuration[configurationKey];


Why not pass timeoutString directly to this method?

benaadams · 2016-02-03T01:38:19Z

Thread count didn't help; got a failed restore second time; third time (without MinWorkerThreads or NO_PARALLEL_TEST_PROJECTS) it was happy...

Might be a dotnet cli issue instead

cesarblum · 2016-02-03T16:02:12Z

@benaadams Yeah, we think it might be dotnet, or the xunit console runner is doing something funny.

cesarblum · 2016-02-03T19:11:12Z

@benaadams Did you see it hang consistently? I've only seen it hang on AppVeyor, can't repro on my box.

benaadams · 2016-02-03T19:35:13Z

No; but also get on/off restore issues with Build when starting with a fresh repo; it seems a little shaky atm compared to the dnu version

(e.g. delete directory; then do fresh get from github)

halter73 · 2016-02-11T22:47:49Z

We should be able to add this for rc2. Do you still think you can write some test cases for this? Thanks!

benaadams · 2016-02-11T22:49:02Z

I'd like advice on test cases; waiting for timeouts and CI never seem to mix well...

benaadams · 2016-02-12T01:04:25Z

To clarify further; the timeouts are in seconds so can't go finer than that; and CI is flakey with timeout numbers ;-/

Write write some see how it goes

halter73 · 2016-02-12T01:13:41Z

I would make timeouts very short (maybe a tenth of a second) and verify that you get 500 responses (header timeout) or the that connection is gracefully (keepalive timeout)/ungracefully(execution timeout). Also verify that ongoing reads get aborted in the event you are in the executing phase.

You can be very generous with the amount of time you wait for the timeouts to trigger. I wouldn't mind if we waited 10 seconds (100 times what they are set to) for the timeouts to trigger. Hopefully that will be long enough for travis/appveyor. If not, we can adjust.

halter73 · 2016-02-12T01:18:38Z

Oh. And this isn't urgent. Feel free to wait until we get AppVeyor and Travis passing again (maybe someday 😞) so you can verify the timeouts work.

benaadams · 2016-02-12T01:21:30Z

Obviously I need to get it in before #623 - think of the merge! ;-)

benaadams · 2016-02-24T01:49:40Z

/cc @halter73 away for a week; so won't be able to rebase or make changes during that time.

Tratcher · 2016-03-07T20:15:53Z

src/Microsoft.AspNetCore.Server.Kestrel/IKestrelServerInformation.cs

 using Microsoft.AspNetCore.Server.Kestrel.Filter;

 namespace Microsoft.AspNetCore.Server.Kestrel
 {
    public interface IKestrelServerInformation
    {
+        TimeSpan ExecutionTimeout { get; set; }


KeepAlive and HeadersComplete are reasonable, but I don't think we need or want ExecutionTimeout at the server level.
A) The same functionality can be provided in app/middleware code.
B) The timeout is un-enforceable. You can't force an app to abandon a request, you can only close the socket and trip the token. The RequestAborted CancellationToken is at best a hint. There are no thread aborts like v4.
C) There are known scenarios this breaks with long running requests. It's one more setting on the server to hunt down and fiddle with when that happens.

benaadams · 2016-05-24T00:52:07Z

@muratg @halter73 @CesarBS going to close this as realistically I won't have enough time to devote to it; the underlying code has changed a lot and there are multiple potential race conditions which have to be guarded against - so will need a lot of testing

MelbourneDeveloper · 2017-03-06T23:20:27Z

I'm pretty confused. Is there currently an ExecutionTimeout in Kestrel? If so, how to we stop it, or change it?

I'm getting a WCF timeout that I can't explain unless it's something going on with a Kestrel request timeout.

davidfowl · 2017-03-07T03:18:05Z

There's no execution timeout in kestrel (which isn't possible to do with async requests because if there's no thread running then there's nothing to time out). How would kestrel affect your WCF service? The ASP.NET core module has a 2 minute timeout for requests, maybe you're running into that?

dnfclas added the cla-already-signed label Dec 14, 2015

benaadams force-pushed the timeouts branch from 3dd5a57 to 697554b Compare December 14, 2015 15:14

muratg reviewed Dec 14, 2015
View reviewed changes

benaadams force-pushed the timeouts branch 2 times, most recently from 07c3768 to 13dc4ef Compare December 15, 2015 03:08

benaadams mentioned this pull request Jan 11, 2016

Add keep-alive timeout parameter #464

Closed

cesarblum reviewed Jan 11, 2016
View reviewed changes

src/Microsoft.AspNet.Server.Kestrel/Http/Frame.cs

{

SocketInput?.AbortAwaiting();

}

}

Copy link

Contributor

cesarblum Jan 11, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: add newline

benaadams force-pushed the timeouts branch 2 times, most recently from 6dfcabc to c8566a2 Compare January 12, 2016 03:18

benaadams force-pushed the timeouts branch from 406e36a to 16a7bda Compare January 12, 2016 03:54

benaadams force-pushed the timeouts branch from 9804088 to 08d9c58 Compare January 12, 2016 23:29

benaadams mentioned this pull request Jan 12, 2016

SocketOutput exception completes all waiting tasks #567

Merged

benaadams force-pushed the timeouts branch from 08d9c58 to bd8910d Compare January 12, 2016 23:32

halter73 reviewed Jan 13, 2016
View reviewed changes

benaadams reviewed Jan 13, 2016
View reviewed changes

muratg reviewed Feb 3, 2016
View reviewed changes

benaadams changed the title ~~[Wip] Add timeouts~~ [Needs Tests] Add timeouts Feb 12, 2016

benaadams mentioned this pull request Feb 23, 2016

Return different status codes depending on condition #653

Closed

Tratcher reviewed Mar 7, 2016
View reviewed changes

muratg modified the milestones: 1.0.0, 1.0.0-rc2 Apr 8, 2016

benaadams closed this May 24, 2016

benaadams mentioned this pull request Jun 11, 2016

Limit size of memory buffer when reading request (#304) #912

Merged

22 tasks

benaadams deleted the timeouts branch March 27, 2018 05:14

guardrex mentioned this pull request May 8, 2019

Alternative for setting request timeouts for in-process hosting dotnet/AspNetCore.Docs#12330

Closed

effyteva mentioned this pull request May 8, 2019

Kestrel execution timeout support dotnet/aspnetcore#10079

Closed

smatsson mentioned this pull request Nov 25, 2021

Trouble Resuming uploads after WiFi dropout due to File locks tusdotnet/tusdotnet#156

Closed

Tratcher mentioned this pull request Dec 22, 2022

Request timeouts middleware dotnet/aspnetcore#45732

Closed

[Needs Tests] Add timeouts #485

[Needs Tests] Add timeouts #485

Uh oh!

Conversation

benaadams commented Dec 14, 2015

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!

benaadams commented Dec 15, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benaadams commented Jan 12, 2016

Uh oh!

cesarblum commented Jan 12, 2016

Uh oh!

benaadams commented Jan 12, 2016

Uh oh!

benaadams commented Jan 13, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benaadams commented Jan 13, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benaadams commented Jan 13, 2016

Uh oh!

benaadams commented Feb 2, 2016

Uh oh!

cesarblum commented Feb 2, 2016

Uh oh!

benaadams commented Feb 2, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benaadams commented Feb 3, 2016

Uh oh!

cesarblum commented Feb 3, 2016

Uh oh!

cesarblum commented Feb 3, 2016

Uh oh!

benaadams commented Feb 3, 2016

Uh oh!

halter73 commented Feb 11, 2016

Uh oh!

benaadams commented Feb 11, 2016

Uh oh!

benaadams commented Feb 12, 2016

Uh oh!

halter73 commented Feb 12, 2016

Uh oh!

halter73 commented Feb 12, 2016

Uh oh!

benaadams commented Feb 12, 2016

Uh oh!

benaadams commented Feb 24, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benaadams commented May 24, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MelbourneDeveloper commented Mar 6, 2017

Uh oh!

davidfowl commented Mar 7, 2017

Uh oh!

Uh oh!

benaadams commented May 24, 2016 •

edited

Loading