Change the IConnection contract to be an IDuplexPipe #1446

davidfowl · 2018-02-14T14:47:47Z

This is a design change that we considered in the past but moved away from while we were discussiong messaging vs streams. We're firmly in the streams camp now so I think we should make this change (it also cleans and a bunch of things and introduces problems 😄 ).

The IConnection implements IDuplexPipe, it's a very simple contract, the IConnection is responsible for writing to the IDuplex pipe that is exposed.
We already changed the HttpConnection to use pipelines internal so the change there was deleting code.
This change is also required to support SignalR on top of TCP.
The server and client dispatching loop look more similar.
We were able to remove all callback handling from this layer.
The event queue is gone, the pipe is single producer and single consumer. The hub connection layer is the only one that has to implement callbacks.
With this change we don't allow running client callbacks in parallel (not sure why we even bothered before) so we were able to remove the event queue.

Quirks:

Completing the pipe is messy, do we still need StopAsync and DisposeAsync?
Who is responsible for completing the PipeReader and PipeWriter exposed by the connection? At a minimum the IConnection impl needs to complete the application sides.
~~No errors if the you write to the connection before it starts or after it is disposed. This is possible but requires wrapping the PipeReader and PipeWriter returned.~~ (solved)
~~The current implementation gives you a null pipe if you you don't call Start first, this can be fixed by making a wrapping PipeWriter and PipeReader.~~ (solved)
IConnection.DisposeAsync doesn't own the receive loop anymore so it returns before the loop ended. This might be fixable if we hook the right events. This also makes it hard to reuse pipes.(not solved)
Today we have an ill defined contract as to which part of the client is reusable, is that part of the IConnection contract? This might make that a bit tricker.

- Decorate the PipeReader and PipeWriter so the reference is always valid. - Make sure StopAsync waits for the Read loop to drain.

davidfowl · 2018-02-14T18:22:15Z

test/Microsoft.AspNetCore.SignalR.Client.Tests/HubConnectionTests.cs

-            {
-                await registration.InvokeAsync(new byte[0]);
-            }
+            var connection = new Mock<IConnection>();


/cc @pakrym this is what it looks like to mock a pipe 😄

You can implement completed IPipeAwaiter very easy, it would make this mock much cleaner.

Maybe we would end up with a complete mock pipe in a bit.

…pp to end" This reverts commit 9a2140b.

mikaelm12 · 2018-02-14T20:18:36Z

src/Microsoft.AspNetCore.Sockets.Client.Http/HttpConnection.cs

@@ -31,24 +32,24 @@ public class HttpConnection : IConnection
        private readonly object _stateChangeLock = new object();

        private volatile IDuplexPipe _transportChannel;


Should we rename away from channel?

mikaelm12 · 2018-02-14T20:19:37Z

src/Microsoft.AspNetCore.Sockets.Client.Http/HttpConnection.PipeReader.cs

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


mikaelm12 · 2018-02-14T20:19:45Z

src/Microsoft.AspNetCore.Sockets.Client.Http/HttpConnection.PipeWriter.cs

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


mikaelm12 · 2018-02-14T20:28:32Z

src/Microsoft.AspNetCore.SignalR.Client.Core/HubConnection.cs

+                                    InvocationRequest irq;
+                                    switch (message)
+                                    {
+                                        case InvocationMessage invocation:


nit: Can we get some new lines in here

This is literally copied from what was here before. Where do you want the newlines and does our other code look like that?

analogrelay

I still need to go a little deeper into connection lifecycle/races etc. but I have some initial comments.

analogrelay · 2018-02-14T18:57:58Z

src/Microsoft.AspNetCore.SignalR.Client.Core/HubConnection.cs

@@ -140,9 +141,13 @@ private void ResetTimeoutTimer()
            using (var memoryStream = new MemoryStream())
            {
                NegotiationProtocol.WriteMessage(new NegotiationMessage(_protocol.Name), memoryStream);
-                await _connection.SendAsync(memoryStream.ToArray(), _connectionActive.Token);
+
+                // TODO: Pass the token when that's available


analogrelay · 2018-02-14T18:59:28Z

src/Microsoft.AspNetCore.SignalR.Client.Core/HubConnection.cs

            {
-                foreach (var message in messages)
+                while (true)


Can you loop on !_connectionActive.IsCancellationRequested? while(true) makes me nervous, even when I feel confident it will exit :)

We should add a couple cancellation checks in the below code as well, maybe after every message

Nah. I’m going to do a pass removing all of the tokens. I hate the fact that it throws an exception for expected behavior.

Most of the pipelines loops look like this FWIW

I hate the fact that it throws an exception for expected behavior.

Only if you use ThrowIfCancellationRequested

That is the contract for cancellation tokens. The operation is cancelled via an exception. Pipelines has an alternative model that I plan to change to once all the code is moved.

analogrelay · 2018-02-14T20:36:28Z

src/Microsoft.AspNetCore.SignalR.Client.Core/Internal/SignalRClientLoggerExtensions.cs

-        private static readonly Action<ILogger, string, Exception> _sendInvocationCompleted =
-            LoggerMessage.Define<string>(LogLevel.Debug, new EventId(6, nameof(SendInvocationCompleted)), "Sending Invocation '{invocationId}' completed.");
+        private static readonly Action<ILogger, string, HubMessage, Exception> _sendInvocationCompleted =
+            LoggerMessage.Define<string, HubMessage>(LogLevel.Debug, new EventId(6, nameof(SendInvocationCompleted)), "Sending Invocation '{invocationId}' completed. {payload}");


Still super unsure about putting message payloads in logs. They potentially have sensitive information in them.

Yea the current logging isn’t great but that’s a good point

We do it in the JS client too. I'm filing an issue for that because I think this it's generally bad. Let's not add it here though. We can add in attribute about the message (type, probably even method name) but not the argument values.

analogrelay · 2018-02-14T20:44:19Z

src/Microsoft.AspNetCore.Sockets.Client.Http/HttpConnection.PipeReader.cs

+
+            public override void AdvanceTo(SequencePosition consumed)
+            {
+                _connection._transportChannel.Input.AdvanceTo(consumed);


This will NRE if the connection hasn't been started. We should put in a check that provides a better error message.

I don’t want to put checks in all of the methods here since some of performance critical. I’ll see what makes sense and update.

catch(NullReferenceException)

I'll just add the check for now and we can fix it when we do moar performance.

analogrelay · 2018-02-14T20:46:57Z

test/Microsoft.AspNetCore.SignalR.Client.Tests/HubConnectionTests.cs

-            {
-                await registration.InvokeAsync(new byte[0]);
-            }
+            var connection = new Mock<IConnection>();


analogrelay · 2018-02-14T20:53:29Z

Today we have an ill defined contract as to which part of the client is reusable, is that part of the IConnection contract? This might make that a bit tricker.

I wonder if maybe IConnection should be non-restartable and we move the restart logic to HubConnection. After all, the main reason is to keep Hub callbacks bound. It would mean HubConnection needs an IConnection-factory rather than an IConnection but I think that's cleaner. It also means the pipelines, transports, etc. all just get dumped when a reconnect happens.

davidfowl · 2018-02-15T01:17:38Z

Agree @anurse but not as part of this change. It also means your paying for negotiation each time. But maybe that’s ok.

mikaelm12 · 2018-02-15T01:24:43Z

src/Microsoft.AspNetCore.Sockets.Client.Http/HttpConnection.cs

+
+        void IDisposable.Dispose()
+        {
+            // We want to hide this since it noops, it's temporary until we remove IDisposable from IDuplexPipe


File this somewhere so it doesn't get overlooked later

https://github.com/dotnet/corefx/issues/27154

mikaelm12 · 2018-02-15T01:25:40Z

test/Microsoft.AspNetCore.SignalR.Client.Tests/HttpConnectionTests.SendAsync.cs

                        Assert.Equal("Cannot send messages when the connection is not in the Connected state.", exception.Message);
                    });
            }

+


nit: Remove new line

analogrelay · 2018-02-15T02:58:13Z

It also means your paying for negotiation each time

Only for WebSockets, for the other transports you have to pay that cost on reconnect anyway. In theory we could try to detect that WebSockets worked and just do a bare WebSocket connection on reconnect, but that would make transport fallback on reconnect tricky. I think the cost is minimal and acceptable.

analogrelay · 2018-02-15T03:03:36Z

#1450

BrennanConroy · 2018-02-15T17:17:54Z

src/Microsoft.AspNetCore.SignalR.Client.Core/HubConnection.cs

+        {
+            await _connection.StopAsync();
+
+            if (_readingTask != null)


I think you have to capture _readingTask before calling _connection.StopAsync otherwise someone could start a new connection in the closed callback and set _readingTask for the new connection

Yea, this part of the code is racy. I took a look last night and since the receive loop moved out of the place that had all of the locks and synchronization it's a bit messy.

davidfowl · 2018-02-21T04:58:26Z

I'll be back with something else

davidfowl added 4 commits February 13, 2018 23:24

WIP

30dcee9

Tests passing

8c8901e

Self PR feedback

26414bb

More tweaks

1809741

- Decorate the PipeReader and PipeWriter so the reference is always valid. - Make sure StopAsync waits for the Read loop to drain.

davidfowl commented Feb 14, 2018

View reviewed changes

davidfowl added 3 commits February 14, 2018 10:34

Added test back

d8c7397

Attempt to clean up complete and disposal by waiting on the app to end

9a2140b

Revert "Attempt to clean up complete and disposal by waiting on the a…

362d345

…pp to end" This reverts commit 9a2140b.

mikaelm12 reviewed Feb 14, 2018

View reviewed changes

analogrelay reviewed Feb 14, 2018

View reviewed changes

mikaelm12 reviewed Feb 15, 2018

View reviewed changes

davidfowl added 2 commits February 14, 2018 20:30

PR feedback

538daec

Revert error checking in PipeReader

841c39b

KPixel mentioned this pull request Feb 15, 2018

[Protocols] Common connection object for client and server #1453

Closed

BrennanConroy reviewed Feb 15, 2018

View reviewed changes

davidfowl closed this Feb 21, 2018

davidfowl mentioned this pull request Feb 21, 2018

Low level IConnection refactoring #1475

Closed

davidfowl deleted the davidfowl/iconnection-pipe branch March 28, 2018 06:05

davidfowl restored the davidfowl/iconnection-pipe branch March 28, 2018 06:05

davidfowl deleted the davidfowl/iconnection-pipe branch March 30, 2018 05:27

		@@ -31,24 +32,24 @@ public class HttpConnection : IConnection
		private readonly object _stateChangeLock = new object();

		private volatile IDuplexPipe _transportChannel;

Change the IConnection contract to be an IDuplexPipe #1446

Change the IConnection contract to be an IDuplexPipe #1446

Conversation

davidfowl commented Feb 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikaelm12 Feb 14, 2018 • 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

analogrelay left a 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

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

analogrelay commented Feb 14, 2018

davidfowl commented Feb 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

analogrelay commented Feb 15, 2018

analogrelay commented Feb 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidfowl commented Feb 21, 2018

davidfowl commented Feb 14, 2018 •

edited

Loading

mikaelm12 Feb 14, 2018 •

edited

Loading