Add response minimum data rate feature #1939

cesarblum · 2017-07-07T00:45:18Z

Alternative to #1937 that doesn't kill perf.

Instead of piping writes to a loop, it keeps track of each write's timeout in a queue of timeouts.

@natemcmaster I've addressed your feedback here (except the bit about connection state in ResponseTests, I'm currently working on that).

cesarblum · 2017-07-07T00:47:20Z

Perf:

dev: 1,196,131.76 RPS
PR: 1,191,081.72

natemcmaster · 2017-07-07T17:26:24Z

src/Microsoft.AspNetCore.Server.Kestrel.Core/Internal/FrameConnection.cs

+                        TimeSpan.FromSeconds(size / minResponseDataRate.BytesPerSecond).Ticks);
+                    var timeoutTimestamp = (_writeTimingTimeouts.Count == 0
+                            ? _lastTimestamp
+                            : _writeTimingTimeouts.Last()) + timeoutTicks;


This would be clearer if timeoutTimestamp += timeoutTicks was on its own line. I missed it at first because it looks like it's part of the false condition.

Also, I believe the call to Last() is going to be expensive. It calls a Queue.GetEnumerator() and walks the collection to the end.

halter73 · 2017-07-07T18:34:05Z

src/Microsoft.AspNetCore.Server.Kestrel.Core/Features/IHttpMinRequestBodyDataRateFeature.cs

@@ -4,7 +4,7 @@
 namespace Microsoft.AspNetCore.Server.Kestrel.Core.Features
 {
    /// <summary>
-    /// Represents a minimum data rate for the request body of an HTTP request.
+    /// Feature to set the minimum data rate at which the the request body should be received.


I think I would go with "should be received" to "must be sent". I would change the MinDataRate property doc comment to match.

halter73 · 2017-07-07T18:34:28Z

src/Microsoft.AspNetCore.Server.Kestrel.Core/Features/IHttpMinResponseDataRateFeature.cs

+namespace Microsoft.AspNetCore.Server.Kestrel.Core.Features
+{
+    /// <summary>
+    /// Feature to set the minimum data rate at which the response should be sent.


I would change "should be sent" to "must be received".

halter73 · 2017-07-07T18:35:14Z

src/Microsoft.AspNetCore.Server.Kestrel.Core/Features/IHttpMinResponseDataRateFeature.cs

+    public interface IHttpMinResponseDataRateFeature
+    {
+        /// <summary>
+        /// The minimum data rate in bytes/second at which the response should be sent.


halter73 · 2017-07-07T18:47:09Z

src/Microsoft.AspNetCore.Server.Kestrel.Transport.Libuv/Internal/LibuvOutputConsumer.cs

+        {
+            // If the pipe writer was closed with a TimeoutException, a write timed out.
+            // Disposing the socket will cancel the pending UvWriteReq.
+            if (ex is TimeoutException)


I think it's reasonable to have the contract be to close the socket immediate if the output/response pipe is completed with any exception. That would allow us to just make this a null check.

halter73 · 2017-07-07T18:49:04Z

test/shared/TestServiceContext.cs

+                AddServerHeader = false,
+                Limits =
+                {
+                    MinResponseDataRate = new MinDataRate(bytesPerSecond: 1, gracePeriod: TimeSpan.MaxValue)


This is super scary. If we have to do this, let's try to make this change to specific tests.

halter73 · 2017-07-07T18:53:26Z

src/Microsoft.AspNetCore.Server.Kestrel.Core/Internal/FrameConnection.cs

-            else
+        }
+
+        private void CheckForReadTimeout(long timestamp)


Nit: I would rename this methods to CheckForReadRateTimeout and CheckForWriteRateTimeout to make it clear that these aren't absolute timeouts.

halter73 · 2017-07-07T19:01:44Z

src/Microsoft.AspNetCore.Server.Kestrel.Core/Internal/FrameConnection.cs

+
+        private void CheckForReadTimeout(long timestamp)
+        {
+            if (TimedOut || _timeoutTimestamp < long.MaxValue)


I think this deserves a comment.

We don't check for read timeouts if there's a _timeoutTimestamp set? Why not? Isn't this taken care of by _readTimingEnabled?

If we really don't want to do this, It would probably be both more correct an easier to read as if (TimedOut || Interlocked.Read(_timeoutTimestamp) != long.MaxValue)

Added comment and a test.

halter73 · 2017-07-07T19:24:42Z

src/Microsoft.AspNetCore.Server.Kestrel.Core/Internal/FrameConnection.cs

+                    // Add Heartbeat.Interval since this can be called right before the next heartbeat.
+                    _writeTimingLastTimeoutTimestamp += timeoutTicks + Heartbeat.Interval.Ticks;
+
+                    _writeTimingTimeouts.Enqueue(_writeTimingLastTimeoutTimestamp);


Instead of maintaining the queue, I would just add to the previous write timeout timestamp. While this could make a timeout take longer than it would with the queue, it's no different than a larger buffer being written.

Sounds good. It was my initial thought, but I figured it would be easier to remove the queue if I got that feedback than add it later 😁

natemcmaster · 2017-07-07T21:48:54Z

src/Microsoft.AspNetCore.Server.Kestrel.Core/Internal/Http/Frame.cs

@@ -1382,6 +1385,15 @@ private IPipe CreateRequestBodyPipe()
                MaximumSizeLow = 1
            });

+        private IPipe CreateResponsePipe()


Is this still used anywhere?

natemcmaster · 2017-07-07T22:11:49Z

What do benchmarks look like that we've dropped the queue ?

halter73 · 2017-07-07T22:19:23Z

src/Microsoft.AspNetCore.Server.Kestrel.Core/Internal/FrameConnection.cs

+
+                    if (_writeTimingWrites == 0)
+                    {
+                        _writeTimingTimeoutTimestamp = _lastTimestamp;


I think you can just add Heartbeat.Interval.Ticks here and not below.

cesarblum · 2017-07-07T22:20:07Z

@natemcmaster Still good, 1,194,528.36 RPS.

natemcmaster

LGTM.

halter73 · 2017-07-08T00:12:31Z

src/Microsoft.AspNetCore.Server.Kestrel.Core/MinDataRate.cs

            {
-                throw new ArgumentOutOfRangeException(nameof(bytesPerSecond), CoreStrings.NonNegativeNumberRequired);
+                throw new ArgumentOutOfRangeException(nameof(bytesPerSecond), CoreStrings.PositiveNumberRequired);


I would like to include something like the following in the exception message. "If you want to disable a minimum rate, use a null MinDataRate instead."

halter73 · 2017-07-08T00:15:53Z

test/shared/TestServiceContext.cs

@@ -1,6 +1,7 @@
 // Copyright (c) .NET Foundation. All rights reserved.
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

+using System;


Nit: remove using.

…t is set.

…tes.

…n rate.

dnfclas added the cla-already-signed label Jul 7, 2017

cesarblum mentioned this pull request Jul 7, 2017

[Bad perf] Add response minimum data rate feature #1937

Closed

cesarblum requested review from halter73, davidfowl, pakrym and natemcmaster July 7, 2017 01:25

natemcmaster reviewed Jul 7, 2017

View reviewed changes

halter73 reviewed Jul 7, 2017

View reviewed changes

cesarblum force-pushed the cesarbs/response-minimum-rate-no-loop branch from 13454c3 to 093a6fe Compare July 7, 2017 19:16

halter73 reviewed Jul 7, 2017

View reviewed changes

natemcmaster reviewed Jul 7, 2017

View reviewed changes

halter73 reviewed Jul 7, 2017

View reviewed changes

natemcmaster approved these changes Jul 7, 2017

View reviewed changes

cesarblum force-pushed the cesarbs/response-minimum-rate-no-loop branch from 89e58b1 to 73c73a2 Compare July 8, 2017 00:03

halter73 reviewed Jul 8, 2017

View reviewed changes

halter73 approved these changes Jul 8, 2017

View reviewed changes

Cesar Blum Silveira added 4 commits July 7, 2017 17:52

Add response minimum data rate feature.

61fda12

PR feedback.

650047e

Undo changes to SampleApp.

eed6b29

Whitespace.

97e07aa

Cesar Blum Silveira added 15 commits July 7, 2017 17:52

Assert connection state in functional test.

cc191c5

Add Frame tests.

3e7a8a5

Don't call Last() on write timeout queue.

7e77f97

Don't disable minimum response rate on tests.

70984b8

Docs language.

dccfa9b

Dispose socket if writer is completed with any error.

75d00f1

Add comment and test on read timeout not being enforced when a timeou…

8b03b1e

…t is set.

Remove write timeout queue and push timeout further on concurrent wri…

899309e

…tes.

Fix functional test.

d1f39d6

Remove dead code.

fb679f2

Add Heartbeat.Interval to write timeout only on first write.

4621e06

Always complete writer with an error (if there is one).

8c8b6c4

Fix functional test.

de7918a

Log when aborting connection due to write timeout.

d793e0d

Change MinDataRate exception message to point out how to disable a mi…

8ae3c46

…n rate.

cesarblum force-pushed the cesarbs/response-minimum-rate-no-loop branch from ca11731 to 8e53ff0 Compare July 8, 2017 00:52

Add HTTPS test.

81d1691

cesarblum force-pushed the cesarbs/response-minimum-rate-no-loop branch from 8e53ff0 to 81d1691 Compare July 8, 2017 00:56

Cesar Blum Silveira added 2 commits July 7, 2017 19:29

Fix deadlock in LibuvOutputConsumerTests.

e0e77b2

Fix functional test on Linux.

b72c0ff

cesarblum merged commit eca4bfe into dev Jul 8, 2017

cesarblum deleted the cesarbs/response-minimum-rate-no-loop branch July 8, 2017 05:37

cesarblum mentioned this pull request Jul 8, 2017

Socket close on Timeout #1771

Closed

cesarblum mentioned this pull request Aug 10, 2017

Add response minimum data rate #1988

Closed

KPixel mentioned this pull request Sep 5, 2017

Hardening the Sockets layer aspnet/SignalR#820

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add response minimum data rate feature #1939

Add response minimum data rate feature #1939

cesarblum commented Jul 7, 2017 •

edited

Loading

cesarblum commented Jul 7, 2017

natemcmaster Jul 7, 2017

halter73 Jul 7, 2017 •

edited

Loading

halter73 Jul 7, 2017

halter73 Jul 7, 2017

halter73 Jul 7, 2017

halter73 Jul 7, 2017

halter73 Jul 7, 2017

halter73 Jul 7, 2017

cesarblum Jul 7, 2017

halter73 Jul 7, 2017

cesarblum Jul 7, 2017

natemcmaster Jul 7, 2017

natemcmaster commented Jul 7, 2017

halter73 Jul 7, 2017

cesarblum commented Jul 7, 2017

natemcmaster left a comment

halter73 Jul 8, 2017

halter73 Jul 8, 2017

Add response minimum data rate feature #1939

Add response minimum data rate feature #1939

Conversation

cesarblum commented Jul 7, 2017 • edited Loading

cesarblum commented Jul 7, 2017

Choose a reason for hiding this comment

halter73 Jul 7, 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

natemcmaster commented Jul 7, 2017

Choose a reason for hiding this comment

cesarblum commented Jul 7, 2017

natemcmaster left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cesarblum commented Jul 7, 2017 •

edited

Loading

halter73 Jul 7, 2017 •

edited

Loading