Rename request body min rate APIs (#1901) #1915

cesarblum · 2017-06-23T00:27:51Z

#1901

halter73 · 2017-06-23T01:38:33Z

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

-        public MinimumDataRate RequestBodyMinimumDataRate { get; set; } = new MinimumDataRate(rate: 1, gracePeriod: TimeSpan.FromSeconds(5));
+        public MinimumDataRate MinRequestBodyDataRate { get; } = new MinimumDataRate { BytesPerSecond = 1, GracePeriod = TimeSpan.FromSeconds(5) };
+
+        public class MinimumDataRate


I don't like having a public nested class.

The feature uses two fields but the options use a custom type? Why is this type needed for options?

@Tratcher MinRequestBodyDataRate.BytesPerSecond matches how the feature is used better than MinRequestBodyDataRateBytesPerSecond. It's just aesthetics though.

halter73 · 2017-06-23T01:39:19Z

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

+                get => _gracePeriod;
+                set
+                {
+                    if (value < TimeSpan.Zero)


I wonder if we should throw for TimeSpan.Zero too, since that's just asking for problems.

It's still a legal value. Means the rate will be enforced right away. It's not recommended, but there's no technical reason to disallow it.

The technical reason to disallow it is because setting it to zero creates a race that could cause any connection to be killed no matter the actual upload rate

Then we should require it to be larger than 1 second.

Or, more precisely, larger than Heartbeat.Interval, which right now is one second.

halter73 · 2017-06-23T01:42:07Z

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

+            get => MinRequestBodyDataRate.GracePeriod;
+            set
+            {
+                if (value < TimeSpan.Zero)


Is there a way to share the validation logic between this and the MinimumDataRate class?

Tratcher · 2017-06-23T15:36:10Z

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

+        double BytesPerSecond { get; set; }
+
+        /// <summary>
+        /// The amount of time to delay enforcement of the minimum data rate.


Starting from when? After the request arrives? The first read?

Will add ", starting at the time data is first read or written." Written as well since this same type will (likely) be used for the response rate.

Tratcher · 2017-06-23T15:40:54Z

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

-        public MinimumDataRate RequestBodyMinimumDataRate { get; set; } = new MinimumDataRate(rate: 1, gracePeriod: TimeSpan.FromSeconds(5));
+        public MinimumDataRate MinRequestBodyDataRate { get; } = new MinimumDataRate { BytesPerSecond = 1, GracePeriod = TimeSpan.FromSeconds(5) };
+
+        public class MinimumDataRate


The feature uses two fields but the options use a custom type? Why is this type needed for options?

cesarblum · 2017-06-24T03:27:06Z

Reverted 5c63a58 to stop the bikeshedding
Moved MinimumDataRate to the base Core namespace so people don't need a weird using when setting it in KestrelServerLimits
Allow 0 bytes/second
Require the grace period to be larger than Heartbeat.Interval

cesarblum · 2017-06-29T17:47:11Z

📢

halter73 · 2017-06-29T19:16:45Z

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

@@ -6,7 +6,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Features
    /// <summary>
    /// Represents a minimum data rate for the request body of an HTTP request.
    /// </summary>
-    public interface IHttpRequestBodyMinimumDataRateFeature
+    public interface IHttpMinRequestBodyDataRateFeature


I'm not sure we should be abbreviating Minimum to Min

After thinking about this for a little bit, I think we should change all usages of "Minimum" in public APIs to "Min" for consistency considering how many limits already use "Max".

So that means the MinimumDataRate class changes to MinDataRate, IHttpMinRequestBodyDataRateFeature.MinimumDataRate changes to IHttpMinRequestBodyDataRateFeature.MinDataRate, etc...

I don't care as much about private/internal names, but I think some of that could also change to match.

I already change the property/param to bytesPerSecond, so that's taken care of. I'll change the type name now.

I went with Min because we already have Max on other properties.

natemcmaster

+1 for MinDataRate over MinimumDataRate. It's more consistent. All other changes look fine to me.

halter73 · 2017-06-29T22:19:36Z

I meant to leave a comment with my approval. I approve this pending changing "Minimum" to "Min" in all public APIs.

cesarblum · 2017-06-29T22:22:02Z

Updated, will merge when CI is green.

…#1901). - IHttpRequestBodyMinimumDataRateFeature -> IHttpMinRequestBodyDataRateFeature - KestrelServerLimits.RequestBodyMinimumDataRate -> MinRequestBodyDataRate - Also rename "rate" param and property of MinimumDataRate to "bytesPerSecond"

…ture.

…aRateFeature." This reverts commit b194c9b.

….Interval.

halter73 · 2017-06-29T23:40:42Z

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

        /// </summary>
        /// <remarks>
        /// Defaults to 1 byte/second with a 5 second grace period.
        /// </remarks>
-        public MinimumDataRate RequestBodyMinimumDataRate { get; set; } = new MinimumDataRate(rate: 1, gracePeriod: TimeSpan.FromSeconds(5));
+        public MinDataRate MinRequestBodyDataRate { get; set; } = new MinDataRate(bytesPerSecond: 1, gracePeriod: TimeSpan.FromSeconds(5));


Change this to match the IIS default. I believe it's 240 bytes/second.

Separate change. Let's discuss it in the office tomorrow.

And here I was trying to avoid creating a new issue 😢

halter73 · 2017-06-29T23:41:16Z

I know I already approved this. Hopeful changing the default is the last thing.

cesarblum requested review from halter73, davidfowl, pakrym, Tratcher and natemcmaster June 23, 2017 00:27

dnfclas added the cla-already-signed label Jun 23, 2017

cesarblum force-pushed the cesarbs/1901 branch 2 times, most recently from b18c827 to b194c9b Compare June 23, 2017 00:30

halter73 suggested changes Jun 23, 2017

View reviewed changes

Tratcher reviewed Jun 23, 2017

View reviewed changes

cesarblum force-pushed the cesarbs/1901 branch from b194c9b to a2fa06c Compare June 24, 2017 03:25

cesarblum force-pushed the cesarbs/1901 branch 3 times, most recently from 778a6a6 to 524fb68 Compare June 29, 2017 17:46

halter73 reviewed Jun 29, 2017

View reviewed changes

natemcmaster approved these changes Jun 29, 2017

View reviewed changes

halter73 approved these changes Jun 29, 2017

View reviewed changes

Cesar Blum Silveira added 6 commits June 29, 2017 16:34

Expose settable properties directly in IHttpMinRequestBodyDataRateFea…

309d7a8

…ture.

Revert "Expose settable properties directly in IHttpMinRequestBodyDat…

dc41033

…aRateFeature." This reverts commit b194c9b.

Move MinimumDataRate class to base Core namespace.

c7678d2

Allow 0 bytesPerScond; require gracePeriod to be larger than Hearbeat…

eb2f350

….Interval.

Rename MinimumDataRate class to MinDataRate.

d2e8a32

cesarblum force-pushed the cesarbs/1901 branch from 47971e1 to d2e8a32 Compare June 29, 2017 23:34

halter73 suggested changes Jun 29, 2017

View reviewed changes

halter73 approved these changes Jun 30, 2017

View reviewed changes

cesarblum merged commit f2061ed into dev Jun 30, 2017

cesarblum deleted the cesarbs/1901 branch June 30, 2017 00:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rename request body min rate APIs (#1901) #1915

Rename request body min rate APIs (#1901) #1915

cesarblum commented Jun 23, 2017 •

edited

Loading

halter73 Jun 23, 2017

Tratcher Jun 23, 2017

cesarblum Jun 23, 2017

halter73 Jun 23, 2017

cesarblum Jun 23, 2017

halter73 Jun 23, 2017

cesarblum Jun 23, 2017

cesarblum Jun 23, 2017

halter73 Jun 23, 2017

Tratcher Jun 23, 2017

cesarblum Jun 23, 2017

Tratcher Jun 23, 2017

cesarblum commented Jun 24, 2017

cesarblum commented Jun 29, 2017

halter73 Jun 29, 2017

halter73 Jun 29, 2017

cesarblum Jun 29, 2017

natemcmaster left a comment

halter73 commented Jun 29, 2017

cesarblum commented Jun 29, 2017

halter73 Jun 29, 2017

cesarblum Jun 29, 2017

halter73 Jun 30, 2017

halter73 Jun 30, 2017

halter73 commented Jun 29, 2017

Rename request body min rate APIs (#1901) #1915

Rename request body min rate APIs (#1901) #1915

Conversation

cesarblum commented Jun 23, 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

cesarblum commented Jun 24, 2017

cesarblum commented Jun 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

natemcmaster left a comment

Choose a reason for hiding this comment

halter73 commented Jun 29, 2017

cesarblum commented Jun 29, 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

halter73 commented Jun 29, 2017

cesarblum commented Jun 23, 2017 •

edited

Loading