Skip to content

Comments

Improve disconnection handling with safe cleanup and logging#956

Merged
mayuki merged 2 commits intomainfrom
feature/FixHubCleanUpExceptionHandling
Jun 25, 2025
Merged

Improve disconnection handling with safe cleanup and logging#956
mayuki merged 2 commits intomainfrom
feature/FixHubCleanUpExceptionHandling

Conversation

@mayuki
Copy link
Member

@mayuki mayuki commented Jun 25, 2025

Add a new test for StreamingHub disconnection to verify exception handling during cleanup.

Add a new test for StreamingHub disconnection to verify exception handling during cleanup.
@mayuki mayuki requested a review from Copilot June 25, 2025 08:50
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR enhances the StreamingHub disconnection sequence by wrapping cleanup calls in a safe, exception-logging helper and adds a new test to verify that exceptions in OnDisconnected and group disposal are logged and do not prevent cleanup.

  • Introduced CleanupSafeAsync calls around OnDisconnected and Group.DisposeAsync in StreamingHub.
  • Added StreamingHubThrowOnDisconnectTest (and supporting wrapper) to assert that errors during disconnect are caught, logged, and the pending-task registry is disposed.
  • Updated existing tests (UnaryServiceTest, StreamingHubDisconnectionTest) to call factory.Initialize() and removed manual logs.Clear().

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
tests/MagicOnion.Server.Tests/UnaryServiceTest.cs Added factory.Initialize(); removed manual logs.Clear()
tests/MagicOnion.Server.Tests/StreamingHubThrowOnDisconnectTest.cs New test validating safe cleanup on disconnect and error logging
tests/MagicOnion.Server.Tests/StreamingHubDisconnectionTest.cs Added factory.Initialize(); removed manual logs.Clear()
src/MagicOnion.Server/Hubs/StreamingHub.cs Wrapped OnDisconnected and group disposal in CleanupSafeAsync with logging
Comments suppressed due to low confidence (5)

tests/MagicOnion.Server.Tests/UnaryServiceTest.cs:23

  • Removing logs.Clear() may lead to test-state leakage across test runs; consider clearing logs after factory.Initialize() to maintain isolation.
        this.logs = factory.Logs;

tests/MagicOnion.Server.Tests/StreamingHubDisconnectionTest.cs:21

  • Without clearing logs, previous test output may persist and cause false positives; re-add logs.Clear() or reset logs after initialization.
        this.logs = factory.Logs;

tests/MagicOnion.Server.Tests/StreamingHubThrowOnDisconnectTest.cs:53

  • Using a fixed Task.Delay can introduce test flakiness; consider polling the condition with a timeout or using a synchronization primitive instead.
        await Task.Delay(100);

tests/MagicOnion.Server.Tests/StreamingHubThrowOnDisconnectTest.cs:26

  • [nitpick] Using Single will throw if zero or multiple matches are found; consider SingleOrDefault with a null check or First if only one is expected.
                var desc = services.Single(x => x.ServiceType == typeof(IRemoteClientResultPendingTaskRegistry));

src/MagicOnion.Server/Hubs/StreamingHub.cs:164

  • [nitpick] Consider adding XML doc comments summarizing that this helper safely invokes cleanup actions and logs any exceptions to aid maintainability.
        static async ValueTask CleanupSafeAsync(Func<StreamingHubBase<THubInterface, TReceiver>, ValueTask> action, StreamingHubBase<THubInterface, TReceiver> hub)

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant