From fd222c37e5401953ac07302a85e1b3b9d2e5c95c Mon Sep 17 00:00:00 2001 From: Marton Balassa <7115274+BalassaMarton@users.noreply.github.com> Date: Mon, 19 Jun 2023 17:00:36 +0200 Subject: [PATCH] refactor(messaging/dotnet):Added IConnectionFactory for correctly implementing disposable pattern --- src/messaging/dotnet/Directory.Build.props | 1 + .../Client/Client/Abstractions/IConnection.cs | 2 -- .../Client/Abstractions/IConnectionFactory.cs | 25 +++++++++++++++ .../src/Client/Client/MessageRouterClient.cs | 4 +-- ...MessageRouterBuilderWebSocketExtensions.cs | 2 +- .../WebSocket/WebSocketConnectionFactory.cs | 31 +++++++++++++++++++ .../Protocol/Json/MessageBufferConverter.cs | 16 +++++----- .../Client/Internal/InProcessConnection.cs | 9 ++++-- .../Internal/InProcessConnectionFactory.cs | 31 +++++++++++++++++++ ...MessageRouterBuilderInProcessExtensions.cs | 2 +- .../Server/Abstractions/IClientConnection.cs | 8 ++++- .../src/Server/Server/MessageRouterServer.cs | 3 +- .../Server/WebSocket/WebSocketConnection.cs | 9 ++++-- .../Client/MessageRouterClient.Tests.cs | 6 +++- .../Server/MessageRouterServer.Tests.cs | 21 ++++++++++++- .../TestUtils/MockClientConnection.cs | 2 +- 16 files changed, 149 insertions(+), 23 deletions(-) create mode 100644 src/messaging/dotnet/src/Client/Client/Abstractions/IConnectionFactory.cs create mode 100644 src/messaging/dotnet/src/Client/Client/WebSocket/WebSocketConnectionFactory.cs create mode 100644 src/messaging/dotnet/src/Host/Client/Internal/InProcessConnectionFactory.cs diff --git a/src/messaging/dotnet/Directory.Build.props b/src/messaging/dotnet/Directory.Build.props index 9c35b7b24..e41bea3ac 100644 --- a/src/messaging/dotnet/Directory.Build.props +++ b/src/messaging/dotnet/Directory.Build.props @@ -4,5 +4,6 @@ enable enable + CS1066;CS1591 \ No newline at end of file diff --git a/src/messaging/dotnet/src/Client/Client/Abstractions/IConnection.cs b/src/messaging/dotnet/src/Client/Client/Abstractions/IConnection.cs index 7b238e010..b972fb9a3 100644 --- a/src/messaging/dotnet/src/Client/Client/Abstractions/IConnection.cs +++ b/src/messaging/dotnet/src/Client/Client/Abstractions/IConnection.cs @@ -14,8 +14,6 @@ namespace MorganStanley.ComposeUI.Messaging.Client.Abstractions; -// TODO: Add IConnectionFactory abstraction to adhere to DI best practices - /// /// Represents a connection that can communicate with the server. /// diff --git a/src/messaging/dotnet/src/Client/Client/Abstractions/IConnectionFactory.cs b/src/messaging/dotnet/src/Client/Client/Abstractions/IConnectionFactory.cs new file mode 100644 index 000000000..573d4af34 --- /dev/null +++ b/src/messaging/dotnet/src/Client/Client/Abstractions/IConnectionFactory.cs @@ -0,0 +1,25 @@ +// Morgan Stanley makes this available to you under the Apache License, +// Version 2.0 (the "License"). You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0. +// +// See the NOTICE file distributed with this work for additional information +// regarding copyright ownership. Unless required by applicable law or agreed +// to in writing, software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express +// or implied. See the License for the specific language governing permissions +// and limitations under the License. + +namespace MorganStanley.ComposeUI.Messaging.Client.Abstractions; + +/// +/// Creates instances of +/// +public interface IConnectionFactory +{ + /// + /// Creates a new instance. + /// + /// + IConnection CreateConnection(); +} \ No newline at end of file diff --git a/src/messaging/dotnet/src/Client/Client/MessageRouterClient.cs b/src/messaging/dotnet/src/Client/Client/MessageRouterClient.cs index 9bd98cd28..117b7cbe1 100644 --- a/src/messaging/dotnet/src/Client/Client/MessageRouterClient.cs +++ b/src/messaging/dotnet/src/Client/Client/MessageRouterClient.cs @@ -25,11 +25,11 @@ namespace MorganStanley.ComposeUI.Messaging.Client; internal sealed class MessageRouterClient : IMessageRouter { public MessageRouterClient( - IConnection connection, + IConnectionFactory connectionFactory, MessageRouterOptions options, ILogger? logger = null) { - _connection = connection; + _connection = connectionFactory.CreateConnection(); _options = options; _logger = logger ?? NullLogger.Instance; } diff --git a/src/messaging/dotnet/src/Client/Client/WebSocket/MessageRouterBuilderWebSocketExtensions.cs b/src/messaging/dotnet/src/Client/Client/WebSocket/MessageRouterBuilderWebSocketExtensions.cs index ecb29e435..44f498f52 100644 --- a/src/messaging/dotnet/src/Client/Client/WebSocket/MessageRouterBuilderWebSocketExtensions.cs +++ b/src/messaging/dotnet/src/Client/Client/WebSocket/MessageRouterBuilderWebSocketExtensions.cs @@ -31,7 +31,7 @@ public static MessageRouterBuilder UseWebSocket( MessageRouterWebSocketOptions options) { builder.ServiceCollection.AddSingleton>(options); - builder.ServiceCollection.AddTransient(); + builder.ServiceCollection.AddSingleton(); return builder; } } \ No newline at end of file diff --git a/src/messaging/dotnet/src/Client/Client/WebSocket/WebSocketConnectionFactory.cs b/src/messaging/dotnet/src/Client/Client/WebSocket/WebSocketConnectionFactory.cs new file mode 100644 index 000000000..6aab9a292 --- /dev/null +++ b/src/messaging/dotnet/src/Client/Client/WebSocket/WebSocketConnectionFactory.cs @@ -0,0 +1,31 @@ +// Morgan Stanley makes this available to you under the Apache License, +// Version 2.0 (the "License"). You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0. +// +// See the NOTICE file distributed with this work for additional information +// regarding copyright ownership. Unless required by applicable law or agreed +// to in writing, software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express +// or implied. See the License for the specific language governing permissions +// and limitations under the License. + +using Microsoft.Extensions.DependencyInjection; +using MorganStanley.ComposeUI.Messaging.Client.Abstractions; + +namespace MorganStanley.ComposeUI.Messaging.Client.WebSocket; + +internal class WebSocketConnectionFactory : IConnectionFactory +{ + public WebSocketConnectionFactory(IServiceProvider serviceProvider) + { + _serviceProvider = serviceProvider; + } + + public IConnection CreateConnection() + { + return ActivatorUtilities.CreateInstance(_serviceProvider); + } + + private readonly IServiceProvider _serviceProvider; +} \ No newline at end of file diff --git a/src/messaging/dotnet/src/Core/Protocol/Json/MessageBufferConverter.cs b/src/messaging/dotnet/src/Core/Protocol/Json/MessageBufferConverter.cs index d1441b590..8b2805848 100644 --- a/src/messaging/dotnet/src/Core/Protocol/Json/MessageBufferConverter.cs +++ b/src/messaging/dotnet/src/Core/Protocol/Json/MessageBufferConverter.cs @@ -28,13 +28,15 @@ internal class MessageBufferConverter : JsonConverter return null; case JsonTokenType.String: - { - var length = reader.HasValueSequence ? checked((int)reader.ValueSequence.Length) : reader.ValueSpan.Length; - var buffer = MessageBuffer.GetBuffer(length); - length = reader.CopyString(buffer); + { + var length = reader.HasValueSequence + ? checked((int) reader.ValueSequence.Length) + : reader.ValueSpan.Length; + var buffer = MessageBuffer.GetBuffer(length); + length = reader.CopyString(buffer); - return new MessageBuffer(buffer, length); - } + return new MessageBuffer(buffer, length); + } } throw new JsonException(); @@ -44,4 +46,4 @@ public override void Write(Utf8JsonWriter writer, MessageBuffer value, JsonSeria { writer.WriteStringValue(value.GetSpan()); } -} +} \ No newline at end of file diff --git a/src/messaging/dotnet/src/Host/Client/Internal/InProcessConnection.cs b/src/messaging/dotnet/src/Host/Client/Internal/InProcessConnection.cs index f65895e10..cac80bab3 100644 --- a/src/messaging/dotnet/src/Host/Client/Internal/InProcessConnection.cs +++ b/src/messaging/dotnet/src/Host/Client/Internal/InProcessConnection.cs @@ -35,14 +35,19 @@ ValueTask IClientConnection.ReceiveAsync(CancellationToken cancellation return _clientToServer.Reader.ReadAsync(cancellationToken); } - public ValueTask DisposeAsync() + public ValueTask CloseAsync() { _clientToServer.Writer.TryComplete(); _serverToClient.Writer.TryComplete(); - + return ValueTask.CompletedTask; } + public ValueTask DisposeAsync() + { + return CloseAsync(); + } + ValueTask IConnection.ConnectAsync(CancellationToken cancellationToken = default) { return _server.ClientConnected(this); diff --git a/src/messaging/dotnet/src/Host/Client/Internal/InProcessConnectionFactory.cs b/src/messaging/dotnet/src/Host/Client/Internal/InProcessConnectionFactory.cs new file mode 100644 index 000000000..3f6a17d98 --- /dev/null +++ b/src/messaging/dotnet/src/Host/Client/Internal/InProcessConnectionFactory.cs @@ -0,0 +1,31 @@ +// Morgan Stanley makes this available to you under the Apache License, +// Version 2.0 (the "License"). You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0. +// +// See the NOTICE file distributed with this work for additional information +// regarding copyright ownership. Unless required by applicable law or agreed +// to in writing, software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express +// or implied. See the License for the specific language governing permissions +// and limitations under the License. + +using MorganStanley.ComposeUI.Messaging.Client.Abstractions; +using MorganStanley.ComposeUI.Messaging.Server; + +namespace MorganStanley.ComposeUI.Messaging.Client.Internal; + +internal sealed class InProcessConnectionFactory : IConnectionFactory +{ + private readonly IMessageRouterServer _server; + + public InProcessConnectionFactory(IMessageRouterServer server) + { + _server = server; + } + + public IConnection CreateConnection() + { + return new InProcessConnection(_server); + } +} \ No newline at end of file diff --git a/src/messaging/dotnet/src/Host/Client/Internal/MessageRouterBuilderInProcessExtensions.cs b/src/messaging/dotnet/src/Host/Client/Internal/MessageRouterBuilderInProcessExtensions.cs index 0add6304d..f96dc7cdd 100644 --- a/src/messaging/dotnet/src/Host/Client/Internal/MessageRouterBuilderInProcessExtensions.cs +++ b/src/messaging/dotnet/src/Host/Client/Internal/MessageRouterBuilderInProcessExtensions.cs @@ -28,7 +28,7 @@ public static class MessageRouterBuilderInProcessExtensions /// public static MessageRouterBuilder UseServer(this MessageRouterBuilder builder) { - builder.ServiceCollection.AddTransient(); + builder.ServiceCollection.AddSingleton(); return builder; } diff --git a/src/messaging/dotnet/src/Server/Server/Abstractions/IClientConnection.cs b/src/messaging/dotnet/src/Server/Server/Abstractions/IClientConnection.cs index b96f3d52e..2f514deef 100644 --- a/src/messaging/dotnet/src/Server/Server/Abstractions/IClientConnection.cs +++ b/src/messaging/dotnet/src/Server/Server/Abstractions/IClientConnection.cs @@ -17,7 +17,7 @@ namespace MorganStanley.ComposeUI.Messaging.Server.Abstractions; /// /// Abstraction of a client connected to the Message Router server. /// -public interface IClientConnection : IAsyncDisposable +public interface IClientConnection { /// /// Sends a message to the client. @@ -37,4 +37,10 @@ public interface IClientConnection : IAsyncDisposable /// With name if the connection was closed by either party while sending the request /// With name if the connection was closed due to an unexpected error ValueTask ReceiveAsync(CancellationToken cancellationToken = default); + + /// + /// Invoked by the server to notify the object of a disconnection. + /// + /// + ValueTask CloseAsync(); } \ No newline at end of file diff --git a/src/messaging/dotnet/src/Server/Server/MessageRouterServer.cs b/src/messaging/dotnet/src/Server/Server/MessageRouterServer.cs index 5c973f4f9..5e5cf350c 100644 --- a/src/messaging/dotnet/src/Server/Server/MessageRouterServer.cs +++ b/src/messaging/dotnet/src/Server/Server/MessageRouterServer.cs @@ -36,8 +36,7 @@ public async ValueTask DisposeAsync() { _stopTokenSource.Cancel(); - // TODO: Don't dispose objects that were created by someone else. Signal disconnection using a dedicated method. - await Task.WhenAll(_clients.Values.Select(client => client.Connection.DisposeAsync().AsTask())); + await Task.WhenAll(_clients.Values.Select(client => client.Connection.CloseAsync().AsTask())); } public ValueTask ClientConnected(IClientConnection connection) diff --git a/src/messaging/dotnet/src/Server/Server/WebSocket/WebSocketConnection.cs b/src/messaging/dotnet/src/Server/Server/WebSocket/WebSocketConnection.cs index 5ea2ef8b9..adc9702bb 100644 --- a/src/messaging/dotnet/src/Server/Server/WebSocket/WebSocketConnection.cs +++ b/src/messaging/dotnet/src/Server/Server/WebSocket/WebSocketConnection.cs @@ -82,11 +82,16 @@ public async ValueTask ReceiveAsync(CancellationToken cancellationToken } } - public ValueTask DisposeAsync() + public ValueTask CloseAsync() { _stopTokenSource.Cancel(); - return default; + return ValueTask.CompletedTask; + } + + public ValueTask DisposeAsync() + { + return CloseAsync(); } public async Task HandleWebSocketRequest( diff --git a/src/messaging/dotnet/test/Client.Tests/Client/MessageRouterClient.Tests.cs b/src/messaging/dotnet/test/Client.Tests/Client/MessageRouterClient.Tests.cs index d80af65cf..81af9e7f4 100644 --- a/src/messaging/dotnet/test/Client.Tests/Client/MessageRouterClient.Tests.cs +++ b/src/messaging/dotnet/test/Client.Tests/Client/MessageRouterClient.Tests.cs @@ -11,6 +11,7 @@ // and limitations under the License. using System.Linq.Expressions; +using MorganStanley.ComposeUI.Messaging.Client.Abstractions; using MorganStanley.ComposeUI.Messaging.Protocol; using MorganStanley.ComposeUI.Messaging.Protocol.Messages; using MorganStanley.ComposeUI.Messaging.TestUtils; @@ -682,6 +683,9 @@ public Task DisposeAsync() private MessageRouterClient CreateMessageRouter(MessageRouterOptions? options = null) { - return new MessageRouterClient(_connectionMock.Object, options ?? DefaultOptions); + var connectionFactory = new Mock(); + connectionFactory.Setup(_ => _.CreateConnection()).Returns(_connectionMock.Object); + + return new MessageRouterClient(connectionFactory.Object, options ?? DefaultOptions); } } \ No newline at end of file diff --git a/src/messaging/dotnet/test/Server.Tests/Server/MessageRouterServer.Tests.cs b/src/messaging/dotnet/test/Server.Tests/Server/MessageRouterServer.Tests.cs index 8b49d3eb5..5c6959b99 100644 --- a/src/messaging/dotnet/test/Server.Tests/Server/MessageRouterServer.Tests.cs +++ b/src/messaging/dotnet/test/Server.Tests/Server/MessageRouterServer.Tests.cs @@ -13,6 +13,7 @@ using System.Linq.Expressions; using MorganStanley.ComposeUI.Messaging.Protocol; using MorganStanley.ComposeUI.Messaging.Protocol.Messages; +using MorganStanley.ComposeUI.Messaging.Server.Abstractions; using MorganStanley.ComposeUI.Messaging.TestUtils; using TaskExtensions = MorganStanley.ComposeUI.Testing.TaskExtensions; @@ -347,6 +348,25 @@ await client.SendToServer( Times.Once); } + [Fact] + public async Task When_disposed_it_calls_CloseAsync_on_active_connections() + { + var server = CreateServer(); + var connection = new Mock(); + + connection.SetupSequence(_ => _.ReceiveAsync(It.IsAny())) + .Returns(new ValueTask( + new ConnectRequest())) + .Returns(new ValueTask( + Task.Delay(1000).ContinueWith(_ => (Message)new PublishMessage {Topic = "dummy"}))); + + await server.ClientConnected(connection.Object); + await TaskExtensions.WaitForBackgroundTasksAsync(); + await server.DisposeAsync(); + + connection.Verify(_ => _.CloseAsync(), Times.Once); + } + private MessageRouterServer CreateServer(IAccessTokenValidator? accessTokenValidator = null) => new MessageRouterServer(new MessageRouterServerDependencies(accessTokenValidator)); @@ -354,7 +374,6 @@ private MessageRouterServer CreateServer(IAccessTokenValidator? accessTokenValid private async Task CreateAndConnectClient(MessageRouterServer server) { - var connectResponseReceived = new TaskCompletionSource(); var client = CreateClient(); await server.ClientConnected(client.Object); await client.Connect(); diff --git a/src/messaging/dotnet/test/Server.Tests/TestUtils/MockClientConnection.cs b/src/messaging/dotnet/test/Server.Tests/TestUtils/MockClientConnection.cs index 399f4c968..cde1040eb 100644 --- a/src/messaging/dotnet/test/Server.Tests/TestUtils/MockClientConnection.cs +++ b/src/messaging/dotnet/test/Server.Tests/TestUtils/MockClientConnection.cs @@ -26,7 +26,7 @@ public MockClientConnection() Setup(_ => _.ReceiveAsync(It.IsAny())) .Returns((CancellationToken ct) => _sendChannel.Reader.ReadAsync(ct)); - Setup(_ => _.DisposeAsync()) + Setup(_ => _.CloseAsync()) .Callback( () => { _sendChannel.Writer.TryComplete(); }); }