optimize DistributedLocker

[yileicn]

PR Type

Enhancement

---

Description

• Add separate acquireTimeout parameter for lock acquisition

• Distinguish between lock expiry and acquisition timeout durations

• Pass lock expiry configuration to RedisDistributedLock constructor

• Use acquireTimeout for TryAcquire and TryAcquireAsync operations

---

Diagram Walkthrough

flowchart LR
  A["Lock Method Calls"] -->|"timeout param"| B["RedisDistributedLock<br/>Expiry Configuration"]
  A -->|"acquireTimeout param"| C["TryAcquire/<br/>TryAcquireAsync"]
  B --> D["Lock Expiry Duration"]
  C --> E["Lock Acquisition Duration"]

Loading

File Walkthrough

Relevant files

Enhancement

IDistributedLocker.cs Add acquireTimeout parameter to locker interface                  src/Infrastructure/BotSharp.Abstraction/Infrastructures/IDistributedLocker.cs Added acquireTimeout parameter with default value to both Lock and LockAsync methods Maintains backward compatibility with default parameter values +2/-2      DistributedLocker.cs Implement separate timeout parameters for lock operations src/Infrastructure/BotSharp.Core/Infrastructures/DistributedLocker.cs Added acquireTimeoutInSeconds parameter to both Lock and LockAsync methods Convert acquireTimeoutInSeconds to TimeSpan for use in lock acquisition Pass lock expiry timeout to RedisDistributedLock constructor via options lambda Use acquireTimeout for TryAcquire and TryAcquireAsync calls instead of general timeout Update warning messages to reference acquireTimeout instead of timeout +10/-8

---

[qodo-code-review]

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢	No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
⚪	🎫 No ticket provided Create ticket/issue
Codebase Duplication Compliance
⚪	Codebase context is not defined Follow the guide to enable codebase context checks.
Custom Compliance
🟢	Generic: Meaningful Naming and Self-Documenting Code Objective: Ensure all identifiers clearly express their purpose and intent, making code self-documenting Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Error Handling Objective: To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Logging Practices Objective: To ensure logs are useful for debugging and auditing without exposing sensitive information like PII, PHI, or cardholder data. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
⚪	Generic: Comprehensive Audit Trails Objective: To create a detailed and reliable record of critical system actions for security analysis and compliance. Status: Missing audit logs: New lock acquisition paths do not log who performed the action or detailed outcomes beyond generic info/warnings, making it unclear for audit purposes. Referred Code var @lock = new RedisDistributedLock(resource, redis.GetDatabase(),option => option.Expiry(timeout)); await using (var handle = await @lock.TryAcquireAsync(acquireTimeout)) { if (handle == null) { _logger.LogWarning($"Acquire lock for {resource} failed due to after {acquireTimeout}s timeout."); return false; } await action(); return true; Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Robust Error Handling and Edge Case Management Objective: Ensure comprehensive error handling that provides meaningful context and graceful degradation Status: Edge cases unhandled: Passing default acquire timeout may translate to zero seconds, potentially causing immediate acquisition failure without explicit handling or validation. Referred Code public async Task<bool> LockAsync(string resource, Func<Task> action, int timeoutInSeconds = 30, int acquireTimeoutInSeconds = default) { var timeout = TimeSpan.FromSeconds(timeoutInSeconds); var acquireTimeout = TimeSpan.FromSeconds(acquireTimeoutInSeconds); Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Security-First Input Validation and Data Handling Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities Status: Parameter validation: New parameters (timeout and acquireTimeout) accept any integer including negatives or zero without validation in the interface or implementation shown. Referred Code bool Lock(string resource, Action action, int timeout = 30, int acquireTimeout = default); Task<bool> LockAsync(string resource, Func<Task> action, int timeout = 30, int acquireTimeout = default); Learn more about managing compliance generic rules or creating your own custom rules

Compliance status legend

🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

[qodo-code-review]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
High-level	Default behavior change breaks callers The new acquireTimeout parameter defaults to 0, which is a breaking change from the previous behavior of waiting up to 30 seconds. To ensure backward compatibility, acquireTimeout should default to the value of timeout if not explicitly set. Examples: src/Infrastructure/BotSharp.Core/Infrastructures/DistributedLocker.cs [20-36] public async Task<bool> LockAsync(string resource, Func<Task> action, int timeoutInSeconds = 30, int acquireTimeoutInSeconds = default) { var timeout = TimeSpan.FromSeconds(timeoutInSeconds); var acquireTimeout = TimeSpan.FromSeconds(acquireTimeoutInSeconds); var redis = _services.GetService<IConnectionMultiplexer>(); if (redis == null) { #if !DEBUG _logger.LogInformation($"The Redis server is experiencing issues and is not functioning as expected."); ... (clipped 7 lines) src/Infrastructure/BotSharp.Core/Infrastructures/DistributedLocker.cs [49-65] public bool Lock(string resource, Action action, int timeoutInSeconds = 30, int acquireTimeoutInSeconds = default) { var timeout = TimeSpan.FromSeconds(timeoutInSeconds); var acquireTimeout = TimeSpan.FromSeconds(acquireTimeoutInSeconds); var redis = _services.GetRequiredService<IConnectionMultiplexer>(); if (redis == null) { #if !DEBUG _logger.LogWarning($"The Redis server is experiencing issues and is not functioning as expected."); ... (clipped 7 lines) Solution Walkthrough: Before: public async Task<bool> LockAsync(string resource, Func<Task> action, int timeoutInSeconds = 30, int acquireTimeoutInSeconds = default) { var timeout = TimeSpan.FromSeconds(timeoutInSeconds); // `acquireTimeoutInSeconds` is 0 by default. var acquireTimeout = TimeSpan.FromSeconds(acquireTimeoutInSeconds); var @lock = new RedisDistributedLock(resource, redis.GetDatabase(), option => option.Expiry(timeout)); // `TryAcquireAsync` is called with a timeout of 0, meaning it will not wait. await using (var handle = await @lock.TryAcquireAsync(acquireTimeout)) { if (handle == null) { // This path is taken immediately if the lock is already held. return false; } await action(); return true; } } After: public async Task<bool> LockAsync(string resource, Func<Task> action, int timeoutInSeconds = 30, int acquireTimeoutInSeconds = default) { var timeout = TimeSpan.FromSeconds(timeoutInSeconds); // If `acquireTimeoutInSeconds` is not provided (is default 0), use `timeoutInSeconds` to maintain old behavior. var effectiveAcquireTimeoutSeconds = acquireTimeoutInSeconds == default ? timeoutInSeconds : acquireTimeoutInSeconds; var acquireTimeout = TimeSpan.FromSeconds(effectiveAcquireTimeoutSeconds); var @lock = new RedisDistributedLock(resource, redis.GetDatabase(), option => option.Expiry(timeout)); // `TryAcquireAsync` now waits for the lock, preserving backward compatibility. await using (var handle = await @lock.TryAcquireAsync(acquireTimeout)) { if (handle == null) { return false; } await action(); return true; } } Suggestion importance[1-10]: 9 __ Why: The suggestion correctly identifies a critical, silent breaking change in the default behavior of the locking mechanism, which would likely cause failures in existing code under contention.	High
Learned best practice	Validate inputs and default timeout Validate inputs and make acquireTimeout default to timeout when not provided to avoid zero-second waits; also avoid GetRequiredService followed by null-check since it throws. src/Infrastructure/BotSharp.Core/Infrastructures/DistributedLocker.cs [49-78] public bool Lock(string resource, Action action, int timeoutInSeconds = 30, int acquireTimeoutInSeconds = default) { + if (string.IsNullOrWhiteSpace(resource)) throw new ArgumentException("resource must be provided", nameof(resource)); + if (action is null) throw new ArgumentNullException(nameof(action)); + if (timeoutInSeconds <= 0) throw new ArgumentOutOfRangeException(nameof(timeoutInSeconds)); + if (acquireTimeoutInSeconds < 0) throw new ArgumentOutOfRangeException(nameof(acquireTimeoutInSeconds)); + var timeout = TimeSpan.FromSeconds(timeoutInSeconds); - var acquireTimeout = TimeSpan.FromSeconds(acquireTimeoutInSeconds); + var acquireTimeout = acquireTimeoutInSeconds == default ? timeout : TimeSpan.FromSeconds(acquireTimeoutInSeconds); - var redis = _services.GetRequiredService<IConnectionMultiplexer>(); + var redis = _services.GetService<IConnectionMultiplexer>(); if (redis == null) { #if !DEBUG _logger.LogWarning($"The Redis server is experiencing issues and is not functioning as expected."); #endif action(); return false; } var @lock = new RedisDistributedLock(resource, redis.GetDatabase(), option => option.Expiry(timeout)); using (var handle = @lock.TryAcquire(acquireTimeout)) { if (handle == null) { _logger.LogWarning($"Acquire lock for {resource} failed due to after {acquireTimeout}s timeout."); return false; } - else - { - action(); - return true; - } + + action(); + return true; } } Apply / Chat Suggestion importance[1-10]: 6 __ Why: Relevant best practice - Guard against invalid states for optional parameters and ensure consistent fallback logic in sync path as well.	Low
	Add input validation and defaults Validate resource, action, and ensure a sensible default for acquireTimeout before using them to avoid ArgumentOutOfRangeException and NREs. Default acquireTimeout to timeout when not provided. src/Infrastructure/BotSharp.Core/Infrastructures/DistributedLocker.cs [20-47] public async Task<bool> LockAsync(string resource, Func<Task> action, int timeoutInSeconds = 30, int acquireTimeoutInSeconds = default) { + if (string.IsNullOrWhiteSpace(resource)) throw new ArgumentException("resource must be provided", nameof(resource)); + if (action is null) throw new ArgumentNullException(nameof(action)); + if (timeoutInSeconds <= 0) throw new ArgumentOutOfRangeException(nameof(timeoutInSeconds)); + if (acquireTimeoutInSeconds < 0) throw new ArgumentOutOfRangeException(nameof(acquireTimeoutInSeconds)); + var timeout = TimeSpan.FromSeconds(timeoutInSeconds); - var acquireTimeout = TimeSpan.FromSeconds(acquireTimeoutInSeconds); + var acquireTimeout = acquireTimeoutInSeconds == default ? timeout : TimeSpan.FromSeconds(acquireTimeoutInSeconds); ... await using (var handle = await @lock.TryAcquireAsync(acquireTimeout)) { if (handle == null) { _logger.LogWarning($"Acquire lock for {resource} failed due to after {acquireTimeout}s timeout."); return false; } - + await action(); return true; } } [To ensure code accuracy, apply this suggestion manually] Suggestion importance[1-10]: 5 __ Why: Relevant best practice - Guard against invalid states and null/empty inputs before use, especially for optional parameters and resources.	Low
More

[JackJiang1234]

reviewed