[BED-5971] Let's add timeouts to blocking ops #217
Conversation
…calls to PerformNtlmAuthenticationAsync
…propriate unit tests
…wrapper layers lower
definitelynotagoblin
changed the title
…Computer more async
…hing than real value
|
Warning Rate limit exceeded@definitelynotagoblin has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 8 minutes and 40 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThe changes introduce a new Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Timeout
participant FuncOrTask
Caller->>Timeout: ExecuteWithTimeout(timeout, func, parentToken)
Timeout->>FuncOrTask: Start func/call with CancellationToken
Timeout-->>Timeout: Wait for func completion or timeout
alt Completes before timeout
Timeout-->>Caller: Return Result(success)
else Timeout occurs
Timeout-->>FuncOrTask: Cancel token
Timeout-->>Caller: Return Result(failure, "Timeout")
end
sequenceDiagram
participant Processor
participant Timeout
participant RPC/NetAPI/LDAP
Processor->>Timeout: ExecuteRPCWithTimeout(timeout, lambda)
Timeout->>RPC/NetAPI/LDAP: Start operation with CancellationToken
Timeout-->>Timeout: Wait for completion or timeout
alt Success
Timeout-->>Processor: Return Result(success, data)
else Timeout or Cancel
Timeout-->>Processor: Return Result(failure, "Timeout" or "Cancellation requested")
end
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🔭 Outside diff range comments (1)
src/CommonLib/Processors/CertAbuseProcessor.cs (1)
446-455: Consider making OpenSamServer async to avoid potential deadlocks.The current implementation uses
GetAwaiter().GetResult()which can cause deadlocks in certain synchronization contexts. Since this is a virtual method, consider making it async to maintain the async pattern consistently.-public virtual SharpHoundRPC.Result<ISAMServer> OpenSamServer(string computerName) +public virtual async Task<SharpHoundRPC.Result<ISAMServer>> OpenSamServer(string computerName) { - var result = Timeout.ExecuteRPCWithTimeout(TimeSpan.FromMinutes(2), (_) => SAMServer.OpenServer(computerName)).GetAwaiter().GetResult(); + var result = await Timeout.ExecuteRPCWithTimeout(TimeSpan.FromMinutes(2), (_) => SAMServer.OpenServer(computerName)); if (result.IsFailed) { return SharpHoundRPC.Result<ISAMServer>.Fail(result.SError); } return SharpHoundRPC.Result<ISAMServer>.Ok(result.Value); }This change would require updating the caller on line 354 to await the call:
-var openServerResult = OpenSamServer(computerName); +var openServerResult = await OpenSamServer(computerName);
♻️ Duplicate comments (8)
test/unit/ComputerSessionProcessorTest.cs (2)
248-248: Good simplification of event handler pattern.Same improvement as the previous comment - using
Task.CompletedTaskis more appropriate here than an async lambda.
282-282: Good simplification of event handler pattern.Consistent with the other event handler improvements in this file.
src/CommonLib/Processors/UserRightsAssignmentProcessor.cs (2)
74-74: Consistent with timeout refactoring.Same improvement as the previous comment - part of the centralized timeout handling refactoring.
101-101: Consistent with timeout refactoring.Same improvement as the previous comments - completing the migration to centralized timeout handling.
src/CommonLib/SecurityDescriptor.cs (1)
110-111: Consistent blocking operation documentation.Same excellent documentation pattern as the previous comments, providing clear identification of potential blocking behavior with helpful source code references.
src/CommonLib/Processors/ComputerSessionProcessor.cs (1)
294-319: Good update: Registry abstraction with proper async pattern.The refactored registry access using
SHRegistryKey.Connectwith automatic disposal and async pattern aligns with the abstraction improvements mentioned in past reviews. The logic flow remains the same while leveraging the improved registry abstractions.src/CommonLib/Helpers.cs (1)
296-296: Appropriate use of GetAwaiter().GetResult() for backward compatibility.As mentioned in previous reviews, this pattern maintains the synchronous public API while allowing async implementation underneath.
src/CommonLib/Ntlm/HttpNtlmAuthenticationService.cs (1)
154-154: Socket exhaustion concern remains unaddressed.The previous review raised a valid concern about potential socket exhaustion with HttpClient creation.
🧹 Nitpick comments (12)
.github/workflows/build.yml (1)
3-3: Consider retaining apushtrigger for the default branch to keep CI green after mergeLimiting the workflow to
pull_requestmeans that once a PR is merged, subsequent direct pushes to the default branch (e.g., branch-protection merges, hot-fixes from admins, tag builds, manual pushes) will no longer be validated by CI. This can silently introduce broken builds onmain.A balanced setup keeps fast feedback on PRs while still protecting the default branch:
on: - pull_request + push: + branches: + - main # validate every merge/hot-fix on main + pull_request: # validate every PR + branches: + - '*' # optional – keep default behaviourIf build minutes are a concern, at minimum keep
pushfor the default branch only.Ensure your branch-protection rules still enforce successful workflow runs when merging, otherwise this change will reduce protection on
main.test/unit/HttpNtlmAuthenticationServiceTest.cs (1)
114-127: Consider enabling the commented timeout test.The commented test for
AuthWithChannelBindingAsynctimeout appears to be disabled due to a DNS resolution issue. Consider using a mock or localhost endpoint to enable this test case.src/CommonLib/Processors/LocalGroupProcessor.cs (1)
28-41: Consider async pattern for OpenSamServer method.While the timeout parameter addition and use of
Timeout.ExecuteRPCWithTimeoutis good, usingGetAwaiter().GetResult()can potentially cause deadlocks in certain synchronization contexts. Consider making this method async or ensuring it's only called from async contexts.-public virtual SharpHoundRPC.Result<ISAMServer> OpenSamServer(string computerName, TimeSpan timeout = default) +public virtual async Task<SharpHoundRPC.Result<ISAMServer>> OpenSamServer(string computerName, TimeSpan timeout = default) { if (timeout == default) { timeout = TimeSpan.FromMinutes(2); } - var result = Timeout.ExecuteRPCWithTimeout(timeout, (_) => SAMServer.OpenServer(computerName)).GetAwaiter().GetResult(); + var result = await Timeout.ExecuteRPCWithTimeout(timeout, (_) => SAMServer.OpenServer(computerName)); if (result.IsFailed) { return SharpHoundRPC.Result<ISAMServer>.Fail(result.SError); } return SharpHoundRPC.Result<ISAMServer>.Ok(result.Value); }src/CommonLib/ConnectionPoolManager.cs (2)
5-5: Remove unused using directive.The
System.Runtime.CompilerServicesnamespace doesn't appear to be used in this file. Tuple support doesn't require this namespace.-using System.Runtime.CompilerServices;
148-148: Consider completing the TODO comment.The TODO comment suggests uncertainty about whether
GetDirectoryEntryis a blocking call. This should be verified and documented consistently with other blocking calls in the codebase.Would you like me to help verify whether
GetDirectoryEntryis a blocking external call and update the comment accordingly?Also applies to: 161-163
src/CommonLib/LdapUtils.cs (3)
10-10: Remove unused import.The
System.Security.Cryptographynamespace doesn't appear to be used in this file.-using System.Security.Cryptography;
716-716: Verify the timeout duration for NetAPI calls.The 2-minute timeout for
CallNetWkstaGetInfomight be too long for a simple workstation info query. Consider if a shorter timeout would be more appropriate.Would you like me to search for other NetAPI timeout values in the codebase to ensure consistency?
170-172: Consider adding logging to empty catch blocks.While making empty catch blocks explicit improves clarity, consider adding debug-level logging to help with troubleshooting when these operations fail silently.
} catch { - //pass + //pass - expected to fail sometimes + _log.LogTrace("Failed to resolve SID {Sid} using LDAP binding", sid); }Also applies to: 187-190, 218-221
src/CommonLib/Timeout.cs (1)
22-30: Improve cancellation logic to avoid unnecessary cancellations.The current implementation always cancels the token after
Task.WhenAny, even if the task completed successfully. This could trigger cancellation callbacks unnecessarily.Check if timeout occurred before cancelling:
await Task.WhenAny(task, Task.Delay(timeout, cts.Token)); -cts.Cancel(); if (task.IsCompleted) { + cts.Cancel(); // Cancel the delay task try { return Result<T>.Ok(await task); } catch (OperationCanceledException) { } +} else { + cts.Cancel(); // Cancel the operation due to timeout }Also applies to: 50-58, 87-94, 115-123
src/CommonLib/SMB/SmbScanner.cs (1)
91-93: Simplify return statement construction for consistency.The return statements can be simplified by directly setting the property in the object initializer.
-return SharpHoundRPC.Result<SmbScanInfo>.Ok(new SmbScanInfo(host) { - SigningRequired = required, -}); +return SharpHoundRPC.Result<SmbScanInfo>.Ok(new SmbScanInfo(host) { SigningRequired = required });Apply the same simplification to lines 99-101.
Also applies to: 99-101
test/unit/TimeoutTests.cs (1)
8-9: Remove or uncomment the collection definition attributes.These commented lines should either be removed if not needed, or uncommented if parallel test execution needs to be disabled.
If these tests need to run sequentially, uncomment the attributes. Otherwise, remove these lines entirely.
src/CommonLib/LdapConnectionPool.cs (1)
103-103: Consider making the LDAP request timeout configurable.The 2-minute timeout is hardcoded in multiple places. This should be configurable to allow flexibility for different environments.
Add a timeout configuration property:
+private static readonly TimeSpan LdapRequestTimeout = TimeSpan.FromMinutes(2); -response = (SearchResponse)await connectionWrapper.Connection.SendRequestAsync(searchRequest, TimeSpan.FromMinutes(2)); +response = (SearchResponse)await connectionWrapper.Connection.SendRequestAsync(searchRequest, LdapRequestTimeout);Even better, make it configurable via
LdapConfig.Also applies to: 258-258, 471-471, 933-933
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (29)
.github/workflows/build.yml(1 hunks)src/CommonLib/ConnectionPoolManager.cs(10 hunks)src/CommonLib/DirectoryObjects/DirectoryEntryWrapper.cs(7 hunks)src/CommonLib/Extensions.cs(11 hunks)src/CommonLib/Helpers.cs(1 hunks)src/CommonLib/ILdapUtils.cs(1 hunks)src/CommonLib/IRegistryKey.cs(1 hunks)src/CommonLib/LdapConnectionPool.cs(32 hunks)src/CommonLib/LdapUtils.cs(30 hunks)src/CommonLib/Ntlm/HttpNtlmAuthenticationService.cs(4 hunks)src/CommonLib/Processors/CertAbuseProcessor.cs(2 hunks)src/CommonLib/Processors/ComputerSessionProcessor.cs(3 hunks)src/CommonLib/Processors/DCLdapProcessor.cs(1 hunks)src/CommonLib/Processors/DomainTrustProcessor.cs(1 hunks)src/CommonLib/Processors/GPOLocalGroupProcessor.cs(1 hunks)src/CommonLib/Processors/LocalGroupProcessor.cs(9 hunks)src/CommonLib/Processors/PortScanner.cs(2 hunks)src/CommonLib/Processors/SmbProcessor.cs(1 hunks)src/CommonLib/Processors/UserRightsAssignmentProcessor.cs(3 hunks)src/CommonLib/SMB/SmbScanner.cs(12 hunks)src/CommonLib/SecurityDescriptor.cs(2 hunks)src/CommonLib/Timeout.cs(1 hunks)test/unit/CommonLibHelperTests.cs(0 hunks)test/unit/ComputerSessionProcessorTest.cs(3 hunks)test/unit/Facades/MockLdapUtils.cs(1 hunks)test/unit/HttpNtlmAuthenticationServiceTest.cs(2 hunks)test/unit/LocalGroupProcessorTest.cs(14 hunks)test/unit/PortScannerTest.cs(1 hunks)test/unit/TimeoutTests.cs(1 hunks)
💤 Files with no reviewable changes (1)
- test/unit/CommonLibHelperTests.cs
🧰 Additional context used
🧬 Code Graph Analysis (9)
src/CommonLib/Processors/SmbProcessor.cs (1)
src/CommonLib/Timeout.cs (1)
Timeout(8-179)
src/CommonLib/Processors/DCLdapProcessor.cs (1)
src/CommonLib/Timeout.cs (1)
Timeout(8-179)
src/CommonLib/Processors/GPOLocalGroupProcessor.cs (2)
src/CommonLib/ILdapUtils.cs (2)
GetDomain(84-84)GetDomain(90-90)src/CommonLib/LdapUtils.cs (3)
GetDomain(488-516)GetDomain(518-546)GetDomain(555-574)
src/CommonLib/Processors/DomainTrustProcessor.cs (3)
src/CommonLib/ILdapUtils.cs (2)
GetDomain(84-84)GetDomain(90-90)src/CommonLib/LdapUtils.cs (3)
GetDomain(488-516)GetDomain(518-546)GetDomain(555-574)test/unit/Facades/MockLdapUtils.cs (2)
GetDomain(696-699)GetDomain(701-704)
src/CommonLib/Processors/CertAbuseProcessor.cs (1)
src/CommonLib/Timeout.cs (1)
Timeout(8-179)
src/CommonLib/Extensions.cs (5)
test/unit/Utils.cs (1)
Extensions(25-84)src/CommonLib/Logging/Logging.cs (3)
ILogger(31-34)Logging(7-20)LogProvider(22-35)src/CommonLib/LdapConnectionPool.cs (14)
Task(59-66)Task(390-420)Task(656-672)Task(679-696)Task(718-810)Task(812-819)Task(821-855)Task(903-958)Task(966-980)IAsyncEnumerable(68-218)IAsyncEnumerable(220-388)IAsyncEnumerable(422-580)LdapConnection(857-890)TimeSpan(582-586)src/CommonLib/Ntlm/LdapNative.cs (2)
T(118-129)LdapConnection(29-253)src/CommonLib/DirectoryObjects/DirectoryEntryWrapper.cs (2)
DirectoryEntryWrapper(10-199)DirectoryEntryWrapper(13-15)
src/CommonLib/Processors/PortScanner.cs (1)
src/CommonLib/Timeout.cs (1)
Timeout(8-179)
test/unit/Facades/MockLdapUtils.cs (2)
src/CommonLib/ILdapUtils.cs (2)
GetDomain(84-84)GetDomain(90-90)src/CommonLib/LdapUtils.cs (3)
GetDomain(488-516)GetDomain(518-546)GetDomain(555-574)
src/CommonLib/LdapConnectionPool.cs (7)
src/CommonLib/LdapResult.cs (6)
LdapResult(4-29)LdapResult(9-12)LdapResult(14-16)LdapResult(18-20)LdapResult(22-24)LdapResult(26-28)src/CommonLib/LdapUtils.cs (22)
IDirectoryObject(1131-1137)Task(102-106)Task(108-137)Task(139-194)Task(196-243)Task(245-261)Task(263-284)Task(286-309)Task(311-327)Task(329-382)Task(384-424)Task(426-479)Task(576-603)Task(605-702)Task(709-724)LdapUtils(29-1403)LdapUtils(73-78)LdapUtils(80-85)GetDomain(488-516)GetDomain(518-546)GetDomain(555-574)Dispose(1139-1141)src/CommonLib/LdapQueryParameters.cs (3)
GetQueryInfo(43-46)LdapQueryParameters(7-47)LdapQueryParameters(20-23)src/CommonLib/ConnectionPoolManager.cs (3)
Task(74-78)Task(94-106)Dispose(175-182)src/CommonLib/LdapConnectionWrapper.cs (4)
LdapConnectionWrapper(6-96)LdapConnectionWrapper(17-24)GetSearchBase(35-55)SaveContext(66-80)src/CommonLib/Ntlm/LdapNative.cs (2)
Dispose(246-252)LdapConnection(29-253)src/CommonLib/LdapConfig.cs (1)
GetPort(20-31)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (38)
src/CommonLib/DirectoryObjects/DirectoryEntryWrapper.cs (1)
23-28: LGTM! Excellent documentation of blocking operations.These comments effectively identify all blocking external calls to
_entry.Properties, which is valuable for the timeout management initiative. The consistent annotation across all property access points will help with future timeout wrapper implementations.Also applies to: 41-42, 57-59, 72-73, 89-90, 141-142, 173-174, 179-180, 187-189, 195-196
src/CommonLib/Extensions.cs (2)
4-4: LGTM! Proper import addition for LDAP protocols.The new
using System.DirectoryServices.Protocols;import is needed for theLdapConnectionandDirectoryRequest/DirectoryResponsetypes used in the new extension method.
212-231: LGTM! Excellent async modernization of LDAP operations.The
SendRequestAsyncextension method properly wraps the legacy APM pattern (BeginSendRequest/EndSendRequest) into a modern Task-based async method with timeout support. The implementation usingTask.Factory.FromAsyncis the standard approach for this conversion and enables consistent async LDAP operations throughout the codebase.src/CommonLib/ILdapUtils.cs (1)
88-88: LGTM! Minor documentation consistency improvement.The capitalization fix makes the parameter description consistent with standard documentation conventions.
src/CommonLib/Processors/DCLdapProcessor.cs (2)
57-57: LGTM! Proper migration to standardized timeout handling.The replacement of
Helpers.ExecuteRPCWithTimeoutwithTimeout.ExecuteRPCWithTimeoutaligns with the broader effort to standardize timeout and cancellation handling across the codebase. The method signature and timeout parameter usage are correct.
61-61: LGTM! Consistent timeout method migration.The replacement follows the same pattern as line 57, properly migrating from the deprecated
Helperstimeout method to the new centralizedTimeoutclass implementation.src/CommonLib/Processors/SmbProcessor.cs (1)
34-34: LGTM! Proper timeout standardization implementation.The replacement of
Helpers.ExecuteRPCWithTimeoutwithTimeout.ExecuteRPCWithTimeoutis consistent with the broader refactoring effort. The lambda function correctly passes the cancellation token to_smbScanner.ScanHost, enabling proper timeout and cancellation support for SMB scanning operations.src/CommonLib/Processors/GPOLocalGroupProcessor.cs (1)
69-73: LGTM - Improved variable naming for clarity.The variable rename from
dtodomainResultmakes the code more readable and self-documenting. This is a good practice for maintaining code clarity.src/CommonLib/Processors/DomainTrustProcessor.cs (1)
34-40: Excellent improvement - replaced exception-based control flow with boolean return check.This change correctly uses the boolean return value from
GetDomain()instead of relying on exception handling. SinceGetDomain()already handles exceptions internally and returns false on failure, this approach is more efficient and cleaner. The past review comments correctly identified that the try-catch was redundant.test/unit/ComputerSessionProcessorTest.cs (1)
229-229: Good simplification of event handler pattern.Replacing the async lambda with a synchronous lambda returning
Task.CompletedTaskis more efficient and clearer since no actual async work is being performed inside the handler.src/CommonLib/Processors/UserRightsAssignmentProcessor.cs (1)
56-56: LGTM - Centralized timeout handling.The replacement of
Helpers.ExecuteRPCWithTimeoutwithTimeout.ExecuteRPCWithTimeoutaligns with the broader refactoring to centralize timeout functionality in the newTimeoututility class. This improves code organization and consistency across the codebase.src/CommonLib/SecurityDescriptor.cs (2)
87-88: Excellent documentation of blocking operations.The added comment clearly identifies this as a potentially blocking external call, which is valuable for understanding performance implications and the need for timeout handling in the broader context of this PR.
96-97: Detailed documentation with helpful references.The comments provide excellent context about potential blocking behavior with specific references to the .NET runtime source code. This level of documentation helps developers understand the underlying operations and their performance characteristics.
Also applies to: 103-104
src/CommonLib/Processors/CertAbuseProcessor.cs (1)
368-368: LGTM - proper async timeout implementation.The async/await pattern with timeout wrapper is correctly implemented. The 2-minute timeout is reasonable for RPC operations.
test/unit/Facades/MockLdapUtils.cs (1)
696-704: LGTM - correct mock implementation for updated interface.The changes properly implement the mock behavior to match the updated
GetDomainmethod signatures. Returningfalsewithdomain = nullis the appropriate mock behavior for these methods and aligns with the interface contract shown in the relevant code snippets.test/unit/LocalGroupProcessorTest.cs (5)
32-32: LGTM: Mock setups correctly updated for new timeout parameter.All mock setups have been consistently updated to include the new
TimeSpan timeoutparameter that was added to theOpenSamServermethod signature.Also applies to: 60-60, 187-187, 208-208, 229-229, 250-250, 271-271, 294-294, 317-317, 346-346
123-123: Good improvement: Using Assert.Null for null checks.Replacing
Assert.Equal(null, ...)withAssert.Null(...)is more idiomatic and provides clearer test intent.Also applies to: 139-139
165-167: LGTM: Timeout test properly updated.The mock is correctly updated to return a failure result with "Timeout" message, which aligns with the new timeout behavior using the centralized
Timeoututility.
171-173: Good simplification: Event handlers no longer need async lambdas.Removing async lambdas and returning
Task.CompletedTaskdirectly simplifies the event handler code and improves readability.Also applies to: 191-193, 212-214, 233-235, 254-256, 275-277, 298-300, 321-323
334-334: Good improvement: Using Assert.Single for clarity.Using
Assert.Single(rdpGroup.Results)is more expressive than checking the count and provides better failure messages when the assertion fails.test/unit/HttpNtlmAuthenticationServiceTest.cs (4)
27-27: Good refactoring: Converting unnecessary async tests to sync.Removing
asyncfrom test methods that don't perform any awaited operations improves test performance and clarity.Also applies to: 39-39, 51-51, 63-63
35-35: Good improvement: Correct assertion parameter ordering.Placing the expected value first in assertions follows xUnit conventions and provides clearer test failure messages.
Also applies to: 47-47, 59-59, 76-77
81-87: LGTM: Timeout test for EnsureRequiresAuth.The test correctly validates that
EnsureRequiresAuththrows aTimeoutExceptionwith an appropriate message when the operation exceeds the timeout.
91-112: LGTM: Timeout test for AuthWithBadChannelBindingsAsync.The test properly mocks the authentication handler to simulate a delayed response and validates the timeout behavior with the expected exception message.
src/CommonLib/Processors/PortScanner.cs (2)
44-53: Excellent refactoring: Centralized timeout handling.Replacing manual timeout logic with
Timeout.ExecuteWithTimeoutprovides consistent timeout behavior and proper cancellation token support. The error handling correctly uses theIsSuccessproperty and provides appropriate logging and caching.
55-68: Good improvement: Enhanced error handling.The updated success path with proper logging and the exception handling that respects the
throwErrorparameter provides better control over error propagation and debugging capabilities.src/CommonLib/Processors/ComputerSessionProcessor.cs (1)
60-60: LGTM: Consistent timeout utility replacement.Replacing
Helpers.ExecuteNetAPIWithTimeoutwithTimeout.ExecuteNetAPIWithTimeoutaligns with the centralized timeout handling refactoring throughout the codebase.Also applies to: 201-201
src/CommonLib/Processors/LocalGroupProcessor.cs (1)
65-65: LGTM: Consistent timeout utility replacement.All calls to
Helpers.ExecuteRPCWithTimeouthave been consistently replaced withTimeout.ExecuteRPCWithTimeout, maintaining the same timeout behavior while using the new centralized utility.Also applies to: 84-84, 108-108, 129-129, 148-148, 181-181, 202-202, 277-277
src/CommonLib/IRegistryKey.cs (3)
1-9: LGTM! Clean interface extension.The addition of
GetSubKeyNames()method to the interface is well-placed and follows good naming conventions.
11-24: Good use of IDisposable pattern.The implementation of
IDisposableand the private constructor pattern ensures proper resource management and controlled instantiation through the static factory method.
56-58: LGTM! Appropriate mock implementation.Throwing
NotImplementedExceptionis the correct approach for the mock class when the functionality is not needed for testing.src/CommonLib/Ntlm/HttpNtlmAuthenticationService.cs (2)
25-29: Good timeout parameter implementation with sensible default.The 2-minute default timeout is reasonable for NTLM authentication operations, and the parameter allows callers to override when needed.
60-69: Good consistent timeout handling pattern.The timeout wrapper correctly preserves the async operation and provides clear error messaging on timeout.
src/CommonLib/ConnectionPoolManager.cs (1)
32-35: Good refactoring from out parameters to tuples.This change improves readability and makes future async conversion easier, as mentioned in previous reviews.
src/CommonLib/LdapUtils.cs (3)
634-647: Good async conversion with appropriate timeout handling.The conversion to async with a 1-minute timeout for NETBIOS name resolution is reasonable, and the timeout exception is properly logged.
783-789: Well-structured timeout wrapper method.The timeout wrapper method provides a clean abstraction over the async implementation with proper exception propagation.
814-818: Good use of async DNS resolution.The conversion to async DNS operations prevents blocking on network calls.
src/CommonLib/LdapConnectionPool.cs (1)
641-641: Good documentation of blocking external calls.The comments clearly identify blocking operations, which helps with understanding potential performance bottlenecks and areas for future async improvements.
Also applies to: 734-734, 791-791, 907-907
…rnally to follow the Task pattern
This seems reasonable let's give it a shot. I'm curious how CheckPort will respond to the non-routable IP Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
This reverts commit b41bd62.
Description
Adding timeout wrapper to many potentially hanging IO processes, especially where unmanaged code is involved.
Motivation and Context
BED-5971
BED-5972
How Has This Been Tested?
GOAD
ESC 10 lab
Screenshots (if appropriate):
Types of changes
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests
Chores