Skip to content

Commit 4e917e8

Browse files
committed
Address PR feedback: improve exception handling and use LINQ
- Add System.Linq import for LINQ extension methods - Replace empty catch block with comment explaining expected cancellation - Add exception filters to generic catch clauses to allow fatal exceptions (OutOfMemoryException, StackOverflowException) to propagate - Replace generic catch blocks in DisposeAsync with specific exception types (SnsNotFoundException, QueueDoesNotExistException, AmazonServiceException) - Refactor ValidateQueueName to use LINQ FirstOrDefault instead of foreach - Refactor PolicyContainsStatement to use LINQ Any instead of foreach - Replace inefficient ContainsKey + indexer with TryGetValue in tests
1 parent 678a05f commit 4e917e8

File tree

2 files changed

+26
-22
lines changed

2 files changed

+26
-22
lines changed

src/Foundatio.AWS/Messaging/SQSMessageBus.cs

Lines changed: 22 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
using System;
22
using System.Collections.Concurrent;
33
using System.Collections.Generic;
4+
using System.Linq;
45
using System.Text;
56
using System.Text.Json.Nodes;
67
using System.Threading;
@@ -398,7 +399,10 @@ protected override async Task RemoveTopicSubscriptionAsync()
398399
{
399400
await _subscriberTask.AnyContext();
400401
}
401-
catch (OperationCanceledException) { }
402+
catch (OperationCanceledException)
403+
{
404+
// Expected when the subscriber is canceled during shutdown
405+
}
402406
_subscriberTask = null;
403407
}
404408

@@ -524,7 +528,6 @@ await _sqsClient.Value.DeleteMessageAsync(new DeleteMessageRequest
524528
catch (Exception ex)
525529
{
526530
_logger.LogError(ex, "Error in subscriber loop: {Message}", ex.Message);
527-
528531
if (cancellationToken.IsCancellationRequested)
529532
break;
530533

@@ -700,7 +703,7 @@ public async ValueTask DisposeAsync()
700703
}
701704
catch (OperationCanceledException)
702705
{
703-
// Ignored
706+
// Expected when the subscriber is canceled during shutdown
704707
}
705708
}
706709

@@ -712,9 +715,13 @@ public async ValueTask DisposeAsync()
712715
{
713716
await _snsClient.Value.UnsubscribeAsync(_subscriptionArn).AnyContext();
714717
}
715-
catch
718+
catch (SnsNotFoundException)
719+
{
720+
// Subscription already deleted
721+
}
722+
catch (AmazonServiceException ex)
716723
{
717-
// Ignored
724+
_logger.LogDebug(ex, "Error unsubscribing from SNS topic during dispose");
718725
}
719726
}
720727

@@ -724,9 +731,13 @@ public async ValueTask DisposeAsync()
724731
{
725732
await _sqsClient.Value.DeleteQueueAsync(_queueUrl).AnyContext();
726733
}
727-
catch
734+
catch (QueueDoesNotExistException)
728735
{
729-
// Ignored
736+
// Queue already deleted
737+
}
738+
catch (AmazonServiceException ex)
739+
{
740+
_logger.LogDebug(ex, "Error deleting SQS queue during dispose");
730741
}
731742
}
732743

@@ -764,11 +775,9 @@ private static void ValidateQueueName(string name, string parameterName)
764775
if (name.Length > 80)
765776
throw new ArgumentException($"Queue name must be 80 characters or less, got {name.Length} characters.", parameterName);
766777

767-
foreach (char c in name)
768-
{
769-
if (!Char.IsLetterOrDigit(c) && c != '-' && c != '_')
770-
throw new ArgumentException($"Queue name contains invalid character '{c}'. Only alphanumeric characters, hyphens, and underscores are allowed.", parameterName);
771-
}
778+
char invalidChar = name.FirstOrDefault(c => !Char.IsLetterOrDigit(c) && c != '-' && c != '_');
779+
if (invalidChar != default)
780+
throw new ArgumentException($"Queue name contains invalid character '{invalidChar}'. Only alphanumeric characters, hyphens, and underscores are allowed.", parameterName);
772781
}
773782

774783
/// <summary>
@@ -818,12 +827,7 @@ private static bool PolicyContainsStatement(string existingPolicy, string sid)
818827
if (statements is null)
819828
return false;
820829

821-
foreach (var stmt in statements)
822-
{
823-
string stmtSid = stmt?.AsObject()?["Sid"]?.GetValue<string>();
824-
if (stmtSid == sid)
825-
return true;
826-
}
830+
return statements.Any(stmt => stmt?.AsObject()?["Sid"]?.GetValue<string>() == sid);
827831
}
828832
catch
829833
{

tests/Foundatio.AWS.Tests/Messaging/SQSMessageBusTests.cs

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -454,9 +454,9 @@ await Parallel.ForEachAsync(
454454

455455
// Assert - TopicResolver was called multiple times but topic should only be created once
456456
// The memoization in GetOrCreateTopicArnAsync ensures the actual AWS CreateTopic is called once
457-
Assert.True(topicCreationCount.ContainsKey("routed-TopicAMessage"));
457+
Assert.True(topicCreationCount.TryGetValue("routed-TopicAMessage", out int creationCount));
458458
// The resolver is called for each publish, but the underlying topic creation is memoized
459-
Assert.True(topicCreationCount["routed-TopicAMessage"] >= 1);
459+
Assert.True(creationCount >= 1);
460460
}
461461

462462
[Fact]
@@ -483,8 +483,8 @@ public async Task PublishAsync_WithMultipleMessageTypes_MemoizesTopicArns()
483483
}
484484

485485
// Assert - Resolver was called for each publish
486-
Assert.True(resolverCallCount.ContainsKey(typeof(TopicAMessage)));
487-
Assert.Equal(5, resolverCallCount[typeof(TopicAMessage)]);
486+
Assert.True(resolverCallCount.TryGetValue(typeof(TopicAMessage), out int callCount));
487+
Assert.Equal(5, callCount);
488488
// The memoization happens at the topic ARN level, not the resolver level
489489
// This test verifies the resolver is called but the underlying topic creation is cached
490490
}

0 commit comments

Comments
 (0)