Pawelka/channels client #141

moozzyk · 2017-01-20T18:49:02Z

No description provided.

moozzyk · 2017-01-20T18:50:27Z

src/Microsoft.AspNetCore.Sockets.Client/project.json

-    "xmlDoc": true
+    "xmlDoc": true,
+    "compile": {
+      "include": [


We could create a separate common project to share this code but it felt too much for these 3 file (one of which we hope will make it to the channels)

moozzyk · 2017-01-20T18:53:03Z

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

 using Microsoft.Extensions.Logging;

 namespace Microsoft.AspNetCore.Sockets.Client
 {
-    public class Connection : IPipelineConnection
+    public class Connection : IChannelConnection<Message>


I have mixed feelings about Connection being IChannelConnection

exposing Message

stopping the connection is ugly and has loose ends (especially you need to drain the channel in order to make the Completion task complete)

I updated issue #51 but we need more to do more thinking here (or maybe it is fine at the low level?)

Related/rephrased - it is impossible to tell currently what state the client is in when it is disconnected but the channel is not drained

I've been thinking about this a bit and I wonder if there's an upside in exposing the channel interfaces on the client side. The benefit in theory is the ability to share code between client and server but we aren't really doing that. We should make sure the design is right here before going too far with it.

Yes. We need to talk about this but this would be separate from this commit.

moozzyk · 2017-01-20T18:56:15Z

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

+{
+    public struct Message : IDisposable
+    {
+        public Format MessageFormat { get; }


If we want the connection to be IChannel then we should add make using this struct more convenient (e.g. add Message(string data)/Message(byte[] data) ctors etc)

BrennanConroy · 2017-01-20T19:38:35Z

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

                {
-                    while (!cancellationToken.IsCancellationRequested)
+                    Message message;


Same here, we should dispose?

Updated everywhere we Read messages.

BrennanConroy · 2017-01-20T19:45:30Z

test/Microsoft.AspNetCore.Sockets.Client.Tests/ConnectionTests.cs

+{
+    public class ConnectionTests
+    {
+


BrennanConroy · 2017-01-20T20:16:57Z

test/Microsoft.AspNetCore.Sockets.Client.Tests/ConnectionTests.cs

+                    new Message(ReadableBuffer.Create(new byte[] { 1, 1, 2, 3, 5, 8 }).Preserve(), Format.Binary));
+
+                Assert.Equal(sendTcs.Task, await Task.WhenAny(Task.Delay(10000), sendTcs.Task));
+                Assert.Equal(new byte[] { 1, 1, 2, 3, 5, 8 }, sendTcs.Task.Result);


Extract to var data or something

BrennanConroy · 2017-01-20T20:22:05Z

test/Microsoft.AspNetCore.Sockets.Client.Tests/ConnectionTests.cs

+                connection.Output.TryWrite(
+                    new Message(ReadableBuffer.Create(new byte[] { 1, 1, 2, 3, 5, 8 }).Preserve(), Format.Binary));
+
+                Assert.Equal(sendTcs.Task, await Task.WhenAny(Task.Delay(10000), sendTcs.Task));


10 seconds seems a bit much

Will fix. I set it to 10 secs when debugging.

mikaelm12 · 2017-01-20T21:39:07Z

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

@@ -23,7 +24,8 @@ public class LongPollingTransport : ITransport
        private readonly CancellationTokenSource _senderCts = new CancellationTokenSource();
        private readonly CancellationTokenSource _pollCts = new CancellationTokenSource();

-        private IPipelineConnection _pipeline;
+        private IChannelConnection<Message> _toFromConnection;
+


nit: remove new line

BrennanConroy · 2017-01-20T21:40:21Z

test/Microsoft.AspNetCore.Sockets.Client.Tests/ConnectionTests.cs

+                Assert.False(connection.Input.Completion.IsCompleted);
+                connection.Output.TryComplete();
+
+                var completionTask = Task.WhenAny(Task.Delay(1000), connection.Input.Completion);


This seems odd, if we need to read messages to complete the task then Task.Delay will always be returned?

Notice we don't await the task here but below, after we have drained the channel. Perhaps I should call it whenAny task and not the completionTask.

mikaelm12 · 2017-01-20T21:53:11Z

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

-                        await buffer.FlushAsync();
+                        var ms = new MemoryStream();
+                        await response.Content.CopyToAsync(ms);
+                        var message = new Message(ReadableBuffer.Create(ms.ToArray()).Preserve(), Format.Text);


Should this message be disposed as well?

No. We are sending it and the receiver is responsible for disposing it. Otherwise we could dispose something they are using.

mikaelm12 · 2017-01-20T22:30:30Z

test/Microsoft.AspNetCore.Sockets.Client.Tests/LongPollingTransportTests.cs

+        [Fact]
+        public async Task LongPollingTransportStopsPollAndSendLoopsWhenTransportDisposed()
+        {
+            var mockHttpHandler = new Mock<HttpMessageHandler>();


This seems to be repeated. Can we move some of this set up into a helper method?

There is no point in just moving this single line and the configuration varies (the returned response) across the tests so I decided to leave it as is.

davidfowl · 2017-01-23T16:15:05Z

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

-                    // and stash it here because we know the callback will have finished before the end of the await.
-                    var message = await _adapter.ReadMessageAsync(_stream, _binder, cancellationToken);
+                    Message incomingMessage;
+                    if (!_connection.Input.TryRead(out incomingMessage))


This should be in a while loop

Why? No one else is reading so it should always succeed. If it fails (what would be the reason?) is there a guarantee that a subsequent call will succeed because if there isn't then we are in tight infinite loop that can't be completed (unless we also check the cancellation token inside the loop).

Multiple messages can come in. It's not about failure.

I see. You basically want to save a call to await _connection.Input.WaitToReadAsync? Because even now multiple messages will work only we will have to call WaitToReadAsync for each message.

Right, it's the same thing we do on the server.

davidfowl · 2017-01-23T16:21:23Z

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


            // Start sending and polling
-            _poller = Poll(Utils.AppendPath(url, "poll"), _pollCts.Token);
-            _sender = SendMessages(Utils.AppendPath(url, "send"), _senderCts.Token);
+            _poller = Poll(Utils.AppendPath(url, "poll"), _pollCts.Token).ContinueWith(_ => _senderCts.Cancel());


Why do we need to cancel the CTS? Who is it for?

It is to exit the sending loop. They run independently and if polling fails/completes we need to stop the loop that is waiting on the user's requests.

Why not just complete the channel from the outside? Why the cancellation token?

The problem is that even if you TryComplete from outside the send loop will continue to run until it drains the channel. This means that until the channel is drained we will keep sending requests to the server even though the connection is already disconnected. Cancellation token prevents that.
(Note: the channel from application to transport is not currently drained - I think it should be done by the side writing to the channel i.e. HubConnection in case of hubs or the user himself when using Sockets but this is one of the issues I wanted to talk about)
I also think that we actually could use just one cancellation tokens for both send and poll loops.

davidfowl · 2017-01-23T16:22:20Z

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

        }

-        public Task StartAsync(Uri url, IPipelineConnection pipeline)
+        public Task StartAsync(Uri url, IChannelConnection<Message> toFromConnection)


This naming is throwing me off. Can we name things similar to the server? Transport and Application?

It actually helped me (especially at the beginning) but can change it.

davidfowl · 2017-01-25T12:55:17Z

test/Microsoft.AspNetCore.Sockets.Client.Tests/LongPollingTransportTests.cs

+
+                connectionToTransport.Out.Complete();
+
+                Assert.Equal(longPollingTransport.Running, await Task.WhenAny(Task.Delay(1000), longPollingTransport.Running));


Are you going to update it to use the new helpers?

I thought about this. I was thinking about creating a Testing.Common project that contains this kind of things since we would want to use it across different test projects in this repo.

moozzyk · 2017-01-26T06:39:50Z

🆙 📅

davidfowl · 2017-01-26T07:22:27Z

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

        private Task _sender;
        private Task _poller;
+        private readonly CancellationTokenSource _transportCts = new CancellationTokenSource();

        public Task Running { get; private set; }


Who is this task for? It's not part of the interface right?

No, it isn't. It was there and I left it because it is useful for testing but it's a bit weird.

davidfowl

This is fine for now but I'd like to remove all uses of continue with and replace them with async await.

moozzyk · 2017-01-26T15:31:34Z

@davidfowl - This change was to convert from pipelines to channels but I agree we need to redesign the client a little bit and remove ContinueWith() usages.

moozzyk · 2017-01-26T16:02:27Z

I updated #51 with follow up items from this PR.

moozzyk · 2017-01-26T17:47:38Z

Fixed in c997ea8

dnfclas added the cla-already-signed label Jan 20, 2017

moozzyk commented Jan 20, 2017

View reviewed changes

BrennanConroy reviewed Jan 20, 2017

View reviewed changes

moozzyk force-pushed the pawelka/channels-client branch 2 times, most recently from c289461 to 4be02b7 Compare January 20, 2017 20:00

BrennanConroy reviewed Jan 20, 2017

View reviewed changes

mikaelm12 reviewed Jan 20, 2017

View reviewed changes

BrennanConroy reviewed Jan 20, 2017

View reviewed changes

mikaelm12 reviewed Jan 20, 2017

View reviewed changes

davidfowl reviewed Jan 23, 2017

View reviewed changes

moozzyk force-pushed the pawelka/channels-client branch 3 times, most recently from 0c47a50 to 47f20f1 Compare January 25, 2017 06:07

davidfowl reviewed Jan 25, 2017

View reviewed changes

moozzyk and others added 6 commits January 25, 2017 22:16

Sharing channel connections

2040434

Converting pipelines to channels

209b682

Disposing messages

f8d656c

Review feedback

b7e578d

More review feedback

cea1d6c

Use a single token for both poll and send loops

cfc0d3e

moozzyk force-pushed the pawelka/channels-client branch from 47f20f1 to 3f7d3ae Compare January 26, 2017 06:39

Centralizing completing the channel

dd587ac

moozzyk force-pushed the pawelka/channels-client branch from 3f7d3ae to dd587ac Compare January 26, 2017 06:47

davidfowl reviewed Jan 26, 2017

View reviewed changes

davidfowl approved these changes Jan 26, 2017

View reviewed changes

mikaelm12 mentioned this pull request Jan 26, 2017

.NET Client To Do List #51

Closed

18 tasks

moozzyk closed this Jan 26, 2017

moozzyk deleted the pawelka/channels-client branch August 23, 2017 23:17


		connectionToTransport.Out.Complete();

		Assert.Equal(longPollingTransport.Running, await Task.WhenAny(Task.Delay(1000), longPollingTransport.Running));

Pawelka/channels client #141

Pawelka/channels client #141

Conversation

moozzyk commented Jan 20, 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

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

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

moozzyk commented Jan 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidfowl left a comment

Choose a reason for hiding this comment

moozzyk commented Jan 26, 2017

moozzyk commented Jan 26, 2017

moozzyk commented Jan 26, 2017