Fixes #411: Adds option to dispose underlying scoped implementations#412
Conversation
Introduces a `shouldDispose` option to `ScopedCacheClient` and `ScopedFileStorage`. This allows control over whether the underlying cache/storage client is disposed when the scoped client is disposed. By default, the underlying client is not disposed, preserving existing behavior. Adds tests to verify the new disposal behavior.
There was a problem hiding this comment.
Pull Request Overview
Adds a shouldDispose parameter to ScopedCacheClient and ScopedFileStorage constructors to control whether the underlying cache/storage implementations are disposed when the scoped wrapper is disposed. This change addresses issue #411 by providing opt-in disposal control while maintaining backward compatibility.
Key changes:
- Introduces optional
shouldDisposeparameter (defaults tofalsefor backward compatibility) - Updates
Dispose()methods to conditionally dispose underlying implementations - Comprehensive test coverage for both default and explicit disposal behaviors
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/Foundatio/Caching/ScopedCacheClient.cs | Adds shouldDispose parameter and conditional disposal logic to cache client |
| src/Foundatio/Storage/ScopedFileStorage.cs | Adds shouldDispose parameter and conditional disposal logic to file storage |
| tests/Foundatio.Tests/Caching/ScopedCacheClientShouldDisposeTests.cs | Test coverage for cache client disposal behavior scenarios |
| tests/Foundatio.Tests/Storage/ScopedFileStorageShouldDisposeTests.cs | Test coverage for file storage disposal behavior scenarios |
| private bool _isLocked; | ||
| private readonly object _lock = new(); | ||
| private readonly bool _shouldDispose; | ||
|
|
There was a problem hiding this comment.
The constructor signature change removes the default value for the scope parameter. This is a breaking change that could affect existing code that relied on new ScopedCacheClient(client) without specifying a scope. Consider adding an overload that preserves the original signature: public ScopedCacheClient(ICacheClient client, string scope = null, bool shouldDispose = false)
| public ScopedCacheClient(ICacheClient client, string scope = null, bool shouldDispose = false) | |
| : this(client, scope, shouldDispose) | |
| { | |
| } |
| public class ScopedHybridCacheClient : ScopedCacheClient, IHybridCacheClient | ||
| { | ||
| public ScopedHybridCacheClient(IHybridCacheClient client, string scope = null) : base(client, scope) { } | ||
| public ScopedHybridCacheClient(IHybridCacheClient client, string scope = null, bool shouldDispose = false) : base(client, scope, shouldDispose) { } |
There was a problem hiding this comment.
The ScopedHybridCacheClient constructor maintains the default value for scope parameter while the base ScopedCacheClient constructor removes it. This inconsistency could lead to confusion and compilation errors when the base constructor is called.
Introduces scoped cache and file storage implementations that prefix keys/paths with a specified scope. This allows for isolating cache and storage data within a specific context. Adds a `shouldDispose` option to control the disposal of the underlying client.
niemyjski
force-pushed
the
feature/disposable-scoped-implementations
branch
from
b05f480 to
aba1dff
Compare
Allows for overriding the Dispose methods in InMemoryCacheClient and InMemoryFileStorage. This change enables more flexible resource management and customization in derived classes. Updates tests to align with the new virtual Dispose methods and removes unnecessary IDisposable implementations.
Introduces a
shouldDisposeoption toScopedCacheClientandScopedFileStorage.This allows control over whether the underlying cache/storage client is disposed when the scoped client is disposed. By default, the underlying client is not disposed, preserving existing behavior.
Adds tests to verify the new disposal behavior.