chore(messaging): Removed try catch block, added exception OnStopRequ…

…est method
morganstanley · Mar 4, 2024 · 16ee03a · 16ee03a
1 parent 6411f22
commit 16ee03a
Show file tree

Hide file tree

Showing 5 changed files with 32 additions and 38 deletions.
diff --git a/src/messaging/dotnet/src/Client/Client/MessageRouterClient.cs b/src/messaging/dotnet/src/Client/Client/MessageRouterClient.cs
@@ -812,7 +812,10 @@ private async ValueTask TryUnsubscribe(Topic topic)
         }
         catch (MessageRouterException exception)
         {
-            _logger.LogWarning(exception, $"Exception thrown while unsubscribing, topic: {topic.Name}, request id: {requestId}.");
+            if (_logger.IsEnabled(LogLevel.Error))
+            {
+                _logger.LogError(exception, $"Exception thrown while unsubscribing, topic: {topic.Name}, request id: {requestId}.");
+            }
         }
     }
 

diff --git a/src/messaging/dotnet/src/Server/Server/MessageRouterServer.cs b/src/messaging/dotnet/src/Server/Server/MessageRouterServer.cs
@@ -340,8 +340,7 @@ private async Task HandleSubscribeMessage(
 
         try
         {
-            if (!Protocol.Topic.IsValidTopicName(message.Topic))
-                return;
+            Protocol.Topic.Validate(message.Topic);
 
             var topic = _topics.AddOrUpdate(
                 message.Topic,
@@ -367,9 +366,9 @@ await client.Connection.SendAsync(
         }
         catch (Exception exception)
         {
-            if (_logger.IsEnabled(LogLevel.Debug))
+            if (_logger.IsEnabled(LogLevel.Error))
             {
-                _logger.LogDebug($"Exception thrown while handling subscription message: {0}.", exception);
+                _logger.LogError(exception, $"Exception thrown while handling subscription message: {0}.");
             }
 
             try
@@ -384,7 +383,7 @@ await client.Connection.SendAsync(
             }
             finally
             {
-                OnRequestStop(message);
+                OnRequestStop(message, exception);
             }
         }
     }
@@ -464,7 +463,7 @@ await client.Connection.SendAsync(
             }
             finally
             {
-                OnRequestStop(message);
+                OnRequestStop(message, exception);
             }
         }
     }

diff --git a/src/messaging/dotnet/test/Client.Tests/Client/MessageRouterClient.Tests.cs b/src/messaging/dotnet/test/Client.Tests/Client/MessageRouterClient.Tests.cs
@@ -782,7 +782,7 @@ await _connectionMock.SendToClient(
     }
 
     [Fact]
-    public async Task It_log_warning_when_a_subscription_is_disposed_and_the_UnsubscribeResponse_contains_Error()
+    public async Task It_log_error_when_a_subscription_is_disposed_and_the_UnsubscribeResponse_contains_Error()
     {
         await _messageRouter.ConnectAsync();
 
@@ -812,7 +812,7 @@ await _connectionMock.SendToClient(
         _loggerMock
             .Verify(
                 _ => _.Log(
-                    LogLevel.Warning,
+                    LogLevel.Error,
                     It.IsAny<EventId>(),
                     It.Is<It.IsAnyType>((message, _) => message.ToString()!.Contains("Exception thrown while unsubscribing, topic: test-topic")),
                     It.IsAny<MessageRouterException>(),
@@ -827,7 +827,7 @@ public MessageRouterClientTests()
         var connectionFactory = new Mock<IConnectionFactory>();
         connectionFactory.Setup(_ => _.CreateConnection()).Returns(_connectionMock.Object);
         _loggerMock = new Mock<ILogger<MessageRouterClient>>();
-        _loggerMock.Setup(_ => _.IsEnabled(It.IsAny<LogLevel>()));
+        _loggerMock.Setup(_ => _.IsEnabled(It.IsAny<LogLevel>())).Returns(true);
 
         _messageRouter = new MessageRouterClient(connectionFactory.Object, new MessageRouterOptions(), _loggerMock.Object);
         _diagnosticObserver = new MessageRouterDiagnosticObserver(_messageRouter);

diff --git a/src/messaging/js/composeui-messaging-client/src/client/MessageRouterClient.spec.ts b/src/messaging/js/composeui-messaging-client/src/client/MessageRouterClient.spec.ts
@@ -730,9 +730,9 @@ describe("MessageRouterClient", () => {
             await expect(subscribePromise).rejects.toThrowWithName(MessageRouterError, "testError-subscribe");
         });
 
-        it("dispose logs warning when UnsubscribeResponse contains error", async() => {
+        it("dispose logs error when UnsubscribeResponse contains error", async() => {
             const client = new MessageRouterClient(connection, {});
-            const consoleWarnMock = jest.spyOn(console, 'warn').mockImplementation();
+            const consoleErrorMock = jest.spyOn(console, 'error').mockImplementation();
             connection.handle<messages.SubscribeMessage>(
                 "Subscribe",
                 msg => connection.sendToClient<messages.SubscribeResponse>({ type: "SubscribeResponse", requestId: msg.requestId }));
@@ -748,9 +748,9 @@ describe("MessageRouterClient", () => {
             await new Promise(process.nextTick);
             await new Promise(process.nextTick);
 
-            expect(consoleWarnMock).toHaveBeenCalled();
-            expect(consoleWarnMock).toHaveBeenCalledWith("Exception thrown while unsubscribing.", new MessageRouterError("testError-unsubscribe"));
-            consoleWarnMock.mockRestore();
+            expect(consoleErrorMock).toHaveBeenCalled();
+            expect(consoleErrorMock).toHaveBeenCalledWith("Exception thrown while unsubscribing.", new MessageRouterError("testError-unsubscribe"));
+            consoleErrorMock.mockRestore();
         });
     });
 })

diff --git a/src/messaging/js/composeui-messaging-client/src/client/MessageRouterClient.ts b/src/messaging/js/composeui-messaging-client/src/client/MessageRouterClient.ts
@@ -83,16 +83,12 @@ export class MessageRouterClient implements MessageRouter {
         const subscription = topic.subscribe(subscriber);
 
         if (needsSubscription) {
-            try {
-                await this.sendRequest<messages.SubscribeMessage, messages.SubscribeResponse>(
-                    {
-                        requestId: this.getRequestId(),
-                        type: "Subscribe",
-                        topic: topicName
-                    });
-            } catch (error) {
-                throw error;
-            }
+            await this.sendRequest<messages.SubscribeMessage, messages.SubscribeResponse>(
+                {
+                    requestId: this.getRequestId(),
+                    type: "Subscribe",
+                    topic: topicName
+                });
         }
 
         return subscription;
@@ -101,18 +97,14 @@ export class MessageRouterClient implements MessageRouter {
     async publish(topic: string, payload?: MessageBuffer, options?: PublishOptions): Promise<void> {
         await this.checkState();
 
-        try {
-            await this.sendRequest<messages.PublishMessage, messages.PublishResponse>(
-                {
-                    type: "Publish",
-                    requestId: this.getRequestId(),
-                    topic,
-                    payload,
-                    correlationId: options?.correlationId
-                });
-        } catch (error) {
-            throw error;
-        }
+        await this.sendRequest<messages.PublishMessage, messages.PublishResponse>(
+            {
+                type: "Publish",
+                requestId: this.getRequestId(),
+                topic,
+                payload,
+                correlationId: options?.correlationId
+            });
     }
 
     async invoke(endpoint: string, payload?: MessageBuffer, options?: InvokeOptions): Promise<MessageBuffer | undefined> {
@@ -429,7 +421,7 @@ export class MessageRouterClient implements MessageRouter {
                 }
             );
         } catch (error) {
-            console.warn("Exception thrown while unsubscribing.", error);
+            console.error("Exception thrown while unsubscribing.", error);
         }
     }