Add support for timing out poll requests #538

davidfowl · 2017-06-07T10:38:48Z

Default poll request is 110 seconds (like in previous versions of SignalR)
Changed the "stop polling" status code to 205 and the time out
status code to 204
Added LongPollingOptions to HttpSocketOptions

analogrelay

Fix you grammar.

analogrelay · 2017-06-07T15:47:26Z

src/Microsoft.AspNetCore.Sockets.Client/LongPollingTransport.cs

                    {
                        _logger.LogDebug("The server is closing the connection");

                        // Transport closed or polling stopped, we're done
                        break;
                    }
+                    else if (response.StatusCode == HttpStatusCode.NoContent)
+                    {
+                        _logger.LogDebug("The server is timed out poll");


I also do good grammar. It best.

BrennanConroy · 2017-06-07T16:09:25Z

src/Microsoft.AspNetCore.Sockets.Http/HttpConnectionDispatcher.cs

-                        _logger.LogDebug("Connection {connectionId} is already active via {requestId}. Cancelling previous request.", connection.ConnectionId, connection.GetHttpContext().TraceIdentifier);
+                        var existing = connection.GetHttpContext();
+
+                        _logger.LogDebug("Connection {connectionId} is already active via {requestId}. Cancelling previous request.", connection.ConnectionId, existing.TraceIdentifier);


Cancelling... I don't know if I should just give up on this

BrennanConroy · 2017-06-07T16:11:16Z

src/Microsoft.AspNetCore.Sockets.Http/LongPollingOptions.cs

@@ -0,0 +1,11 @@
+using System;


BrennanConroy · 2017-06-07T16:16:09Z

test/Microsoft.AspNetCore.Sockets.Tests/LongPollingTests.cs


            Assert.True(channel.Out.TryComplete());

            await poll.ProcessRequestAsync(context, context.RequestAborted);

-            Assert.Equal(204, context.Response.StatusCode);
+            Assert.Equal(205, context.Response.StatusCode);


Use StatusCodes.Status205ResetContent?

BrennanConroy · 2017-06-07T16:16:23Z

test/Microsoft.AspNetCore.Sockets.Tests/LongPollingTests.cs

+            {
+                await poll.ProcessRequestAsync(context, cts.Token).OrTimeout();
+
+                Assert.Equal(204, context.Response.StatusCode);


Use StatusCodes.Status204NoContent?

davidfowl · 2017-06-07T17:07:13Z

HttpClient is trolling me and I'm trying to figure out why.

moozzyk · 2017-06-07T17:11:09Z

test/Microsoft.AspNetCore.Sockets.Tests/HttpConnectionDispatcherTests.cs

@@ -697,6 +723,7 @@ public class HttpConnectionDispatcherTests
            var connection = manager.CreateConnection();
            var dispatcher = new HttpConnectionDispatcher(manager, new LoggerFactory());
            var context = new DefaultHttpContext();
+            context.Features.Set<IHttpResponseFeature>(new ResponseFeature());


Why do we need it?

Because I started to use RegisterForDispose and the DefaultHttpContext throws. I plan to fix this in HttpAbstractions because it's a troll.

aspnet/HttpAbstractions#864

Thanks - it's clear now. I did not know it threw by default.

moozzyk · 2017-06-07T21:21:43Z

src/Microsoft.AspNetCore.Sockets.Http/HttpConnectionDispatcher.cs

@@ -210,7 +210,7 @@ public HttpConnectionDispatcher(ConnectionManager manager, ILoggerFactory logger
                    await connection.TransportTask;

                    // If the status code is a 205 it means the connection is done
-                    if (context.Response.StatusCode == StatusCodes.Status205ResetContent)
+                    if (context.Response.StatusCode == StatusCodes.Status204NoContent)


Fix the warning above

moozzyk · 2017-06-07T21:22:30Z

src/Microsoft.AspNetCore.Sockets.Http/HttpConnectionDispatcher.cs

@@ -219,7 +219,7 @@ public HttpConnectionDispatcher(ConnectionManager manager, ILoggerFactory logger
                        pollAgain = false;
                    }
                }
-                else if (context.Response.StatusCode == StatusCodes.Status205ResetContent)
+                else if (context.Response.StatusCode == StatusCodes.Status204NoContent)
                {


Is this branch dead now?

moozzyk · 2017-06-07T21:23:57Z

src/Microsoft.AspNetCore.Sockets.Http/Transports/LongPollingTransport.cs

@@ -74,14 +74,16 @@ public LongPollingTransport(CancellationToken timeoutToken, ReadableChannel<byte
                // Case 2
                else if (_timeoutToken.IsCancellationRequested)
                {
-                    _logger.LogInformation("Poll request timed out. Sending 204 response.");
-                    context.Response.StatusCode = StatusCodes.Status204NoContent;
+                    _logger.LogInformation("Poll request timed out. Sending 200 response.");


Fix the comment above still mentioning 205

moozzyk · 2017-06-07T21:24:21Z

src/Microsoft.AspNetCore.Sockets/ConnectionManager.cs

@@ -95,7 +96,7 @@ public void Scan()

            try
            {
-                if (_disposed)
+                if (_disposed || Debugger.IsAttached)


Nah, we're keeping it this time.

moozzyk · 2017-06-07T21:25:07Z

test/Common/TaskExtensions.cs

@@ -18,7 +20,7 @@ public static Task OrTimeout(this Task task, int milliseconds = DefaultTimeout,

        public static async Task OrTimeout(this Task task, TimeSpan timeout, [CallerMemberName] string memberName = null, [CallerFilePath] string filePath = null, [CallerLineNumber] int? lineNumber = null)
        {
-            var completed = await Task.WhenAny(task, Task.Delay(timeout));
+            var completed = await Task.WhenAny(task, Task.Delay(Debugger.IsAttached ? Timeout.InfiniteTimeSpan : timeout));


I remember how much you were against it - but this is so useful when debugging.

Yea, I gave up. We're keeping it. Our product is impossible to debug without it.

- Default poll request is 110 seconds (like in previous versions of SignalR) - Changed the "stop polling" status code to 205 and the time out status code to 204 - Added LongPollingOptions to HttpSocketOptions

- Added support for not timing out while debugging

davidfowl requested review from analogrelay, moozzyk, mikaelm12 and BrennanConroy June 7, 2017 10:38

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

analogrelay approved these changes Jun 7, 2017

View reviewed changes

BrennanConroy reviewed Jun 7, 2017

View reviewed changes

src/Microsoft.AspNetCore.Sockets.Http/LongPollingOptions.cs

@@ -0,0 +1,11 @@

using System;

Copy link

Member

BrennanConroy Jun 7, 2017

Choose a reason for hiding this comment

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

copyright

BrennanConroy reviewed Jun 7, 2017

View reviewed changes

moozzyk reviewed Jun 7, 2017

View reviewed changes

moozzyk approved these changes Jun 7, 2017

View reviewed changes

moozzyk reviewed Jun 7, 2017

View reviewed changes

davidfowl added 6 commits June 7, 2017 23:16

Add support for timing out poll requests

0b9f750

- Default poll request is 110 seconds (like in previous versions of SignalR) - Changed the "stop polling" status code to 205 and the time out status code to 204 - Added LongPollingOptions to HttpSocketOptions

Use 200 with a 0 content length for timeouts.

8f57373

- Added support for not timing out while debugging

Fixed the js client

9d0bafa

Removed useless branch

7165f0c

205 -> 204

fb7b6bb

Fixed tests

a036848

davidfowl force-pushed the davidfowl/timeout-longpolling branch from 283932f to a036848 Compare June 8, 2017 06:37

davidfowl merged commit 523517f into dev Jun 8, 2017

davidfowl deleted the davidfowl/timeout-longpolling branch June 9, 2017 18:19

Add support for timing out poll requests #538

Add support for timing out poll requests #538

Uh oh!

Conversation

davidfowl commented Jun 7, 2017

Uh oh!

analogrelay left a comment

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!

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!

davidfowl commented Jun 7, 2017

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!

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!

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!

Uh oh!