Async refactorings#1513
Merged
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR refactors asynchronous code related to thread dumping and dynamic task result retrieval within the Steeltoe codebase.
- Refactored timeout handling logic in EventPipeThreadDumper to use a dedicated CancellationTokenSource.
- Replaced the direct Result property access with a new GetResult() method in TaskShim and its call sites across multiple connector shims.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/Management/src/Endpoint/Actuators/ThreadDump/EventPipeThreadDumper.cs | Refactored async timeout logic to use an internal CancellationTokenSource and CancelAsync for cleaning up the Task.Delay timer. |
| src/Connectors/src/Connectors/Redis/DynamicTypeAccess/DatabaseInterfaceShim.cs | Updated to use GetResult() for retrieving the task result instead of accessing Result directly. |
| src/Connectors/src/Connectors/Redis/DynamicTypeAccess/ConnectionMultiplexerShim.cs | Similar change applied: using GetResult() to retrieve the task result for consistent behavior. |
| src/Connectors/src/Connectors/RabbitMQ/DynamicTypeAccess/ConnectionFactoryInterfaceShim.cs | Updated retrieval of task result via GetResult() ensuring consistency. |
| src/Connectors/src/Connectors/MongoDb/DynamicTypeAccess/MongoClientInterfaceShim.cs | Consistent update using GetResult() for better clarity in async result extraction. |
| src/Common/src/Common/DynamicTypeAccess/TaskShim.cs | Replaced the Result property with a GetResult() method, impacting multiple shim files for consistency. |
Comments suppressed due to low confidence (3)
src/Management/src/Endpoint/Actuators/ThreadDump/EventPipeThreadDumper.cs:443
- Ensure that CancelAsync() is a valid and well-documented extension method in this codebase, as it is not part of the standard .NET API. Consider using the standard Cancel() method if a custom async cancel behavior is not required.
await timeoutSource.CancelAsync();
src/Common/src/Common/DynamicTypeAccess/TaskShim.cs:26
- [nitpick] The refactor replacing the 'Result' property with the GetResult() method improves clarity; ensure that its exception handling and performance characteristics match the previous behavior.
public TResult GetResult()
src/Connectors/src/Connectors/Redis/DynamicTypeAccess/DatabaseInterfaceShim.cs:20
- [nitpick] Updating to GetResult() enhances consistency across dynamic shims; verify that all similar usages have been updated to prevent potential runtime discrepancies.
return taskShim.GetResult();
2abfca1 to
6a85fe8
Compare
|
TimHess
approved these changes
May 6, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.



Description
Reviewed the Steeltoe codebase against the guidance at https://github.com/davidfowl/AspNetCoreDiagnosticScenarios/blob/master/AsyncGuidance.md.
We can't apply all of the guidance because there's simply no async call stack in Configuration. So we're stuck with sync-over-async there. That unnecessarily blocks a thread, but fortunately doesn't deadlock in ASP.NET Core. Our async-void cases catch all exceptions, so it won't crash the process. What remains are quite some violations in DotNetHeapDump, but changing that is very high risk, so I didn't bother.
We have some violations that only appear in tests, resulting in potential memory leaks, but that's not too important, and there is no easy fix.
Closes #1475.
Quality checklist
If your change affects other repositories, such as Documentation, Samples and/or MainSite, add linked PRs here.