Skip to content

Implement safe disposal pattern in AmqpHandler#307

Merged
OrjanSkotnes merged 2 commits intomainfrom
fix/dispose-objectdisposed-rabbitmq
Oct 30, 2025
Merged

Implement safe disposal pattern in AmqpHandler#307
OrjanSkotnes merged 2 commits intomainfrom
fix/dispose-objectdisposed-rabbitmq

Conversation

@OrjanSkotnes
Copy link
Copy Markdown
Contributor

This pull request enhances the reliability and robustness of the AMQP handler's disposal logic and significantly expands the test coverage for resource cleanup scenarios. The changes ensure that disposing the AMQP handler is safe to call multiple times, gracefully handles edge cases like null or already disposed connections, and logs issues without throwing exceptions. The test suite is updated to verify these behaviors.

Improvements to disposal logic in AmqpHandler:

  • Made DisposeAsync idempotent by introducing a _disposed flag and using Interlocked.Exchange to ensure resources are only cleaned up once.
  • Added safe event unsubscription and resource disposal methods (RunSafe, DisposeSafeAsync, SafelyUnsubscribeConnectionEvents) to handle ObjectDisposedException and log warnings instead of throwing.
  • Updated connection logic and refactored parameter names for clarity.

Expanded and improved test coverage:

  • Added multiple tests to verify that DisposeAsync:
    • Handles already disposed or null connections gracefully.
    • Can be called multiple times without error.
    • Skips event unsubscription when the connection is closed.
    • Handles exceptions during event unsubscription and resource disposal without propagating them.
  • Updated the test fixture to support these scenarios by enabling direct manipulation of the handler's connection and exposing the handler instance.

These changes collectively ensure that the AMQP handler's cleanup is robust, predictable, and well-tested, reducing the risk of resource leaks or unexpected exceptions during shutdown.

Copy link
Copy Markdown

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 implements a robust disposal pattern for the AMQP handler to prevent resource leaks and handle edge cases during cleanup. The changes ensure idempotent disposal, graceful error handling, and comprehensive test coverage for various disposal scenarios.

Key Changes:

  • Introduced idempotent disposal using Interlocked.Exchange with a _disposed flag to prevent duplicate cleanup
  • Added defensive error handling methods (RunSafe, DisposeSafeAsync, SafelyUnsubscribeConnectionEvents) that catch and log exceptions instead of propagating them
  • Expanded test suite with 5 new tests covering edge cases: null connections, closed connections, multiple disposal calls, and exception handling during cleanup

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
KS.Fiks.IO.Client/Amqp/AmqpHandler.cs Implements idempotent disposal pattern with error handling helpers and refactored connection logic
KS.Fiks.IO.Client.Tests/Amqp/AmqpHandlerTests.cs Adds comprehensive tests for disposal edge cases including null connections, closed connections, and exception handling
KS.Fiks.IO.Client.Tests/Amqp/AmqpHandlerFixture.cs Enhances test fixture to support disposal testing by exposing handler instance and enabling connection manipulation via reflection

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread KS.Fiks.IO.Client.Tests/Amqp/AmqpHandlerFixture.cs
Comment thread KS.Fiks.IO.Client.Tests/Amqp/AmqpHandlerFixture.cs Outdated
Copy link
Copy Markdown
Contributor

@jarleborsheim jarleborsheim left a comment

Choose a reason for hiding this comment

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

LGTM 🦖
Men kanskje lurt at noen andre ser på også

Copy link
Copy Markdown

@tobiasla tobiasla left a comment

Choose a reason for hiding this comment

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

LGTM :shipit:

Comment thread KS.Fiks.IO.Client.Tests/Amqp/AmqpHandlerFixture.cs Outdated
@OrjanSkotnes OrjanSkotnes merged commit 61102c8 into main Oct 30, 2025
1 check passed
@OrjanSkotnes OrjanSkotnes deleted the fix/dispose-objectdisposed-rabbitmq branch October 30, 2025 12:10
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.

5 participants