-
Notifications
You must be signed in to change notification settings - Fork 375
Add refactoring-to-async skill #79
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,186 @@ | ||||||||||||||||||||||||||||||||||||||
| --- | ||||||||||||||||||||||||||||||||||||||
| name: refactoring-to-async | ||||||||||||||||||||||||||||||||||||||
| description: Convert synchronous .NET code to async/await, including proper Task propagation, cancellation support, and avoiding common async anti-patterns. Use when converting blocking I/O calls to async, fixing thread pool starvation, or modernizing sync-over-async code. | ||||||||||||||||||||||||||||||||||||||
| --- | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| # Refactoring to Async | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| ## When to Use | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| - Converting synchronous I/O-bound code to async/await | ||||||||||||||||||||||||||||||||||||||
| - Fixing thread pool starvation caused by blocking calls | ||||||||||||||||||||||||||||||||||||||
| - Modernizing legacy `.Result` / `.Wait()` / `.GetAwaiter().GetResult()` patterns | ||||||||||||||||||||||||||||||||||||||
| - Adding `CancellationToken` support to async call chains | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| ## When Not to Use | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| - The code is CPU-bound (async won't help; consider `Parallel.For` or `Task.Run`) | ||||||||||||||||||||||||||||||||||||||
| - The synchronous code has no I/O operations | ||||||||||||||||||||||||||||||||||||||
| - The user wants to parallelize work, not make it async | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| ## Inputs | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| | Input | Required | Description | | ||||||||||||||||||||||||||||||||||||||
| |-------|----------|-------------| | ||||||||||||||||||||||||||||||||||||||
| | Code to refactor | Yes | The synchronous methods to convert | | ||||||||||||||||||||||||||||||||||||||
| | Scope | No | Single method, class, or full call chain | | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| ## Workflow | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| ### Step 1: Identify blocking I/O calls | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| Search for synchronous I/O patterns in the codebase: | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| ```bash | ||||||||||||||||||||||||||||||||||||||
| grep -rn "\.Result\b\|\.Wait()\|\.GetAwaiter()\.GetResult()\|ReadToEnd()\|\.Read()\|\.Write(" --include="*.cs" . | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
| grep -rn "\.Result\b\|\.Wait()\|\.GetAwaiter()\.GetResult()\|ReadToEnd()\|\.Read()\|\.Write(" --include="*.cs" . | |
| grep -rnP "\.Result\b|\.Wait\(\)|\.GetAwaiter\(\)\.GetResult\(\)|ReadToEnd\(\)|\.Read\(|\.Write\(" --include="*.cs" . |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| | `reader.Read()` (DbDataReader) | `await reader.ReadAsync()` | | |
| | `command.ExecuteNonQuery()` (DbCommand) | `await command.ExecuteNonQueryAsync()` | | |
maybe
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| | Error | Fix | | |
| |---|---| | |
| | `CS4032`: `await` in non-async method | Add `async` to the method signature and return `Task` or `Task<T>` | | |
| | `CS0029`: Cannot convert `Task<T>` to `T` | Add `await` before the call | | |
| | `CS0127`: Method returns `Task` but body returns value | Change return type to `Task<T>` | | |
| | `CS1998`: Async method lacks `await` | Remove `async` if no awaits are needed, or the method is genuinely sync | | |
| | Error | Fix | | |
| |---|---| | |
| Error | Fix | | |
| |---|---| | |
| | `CS4032`: `await` in non-async method | Add `async` to the method signature and return `Task` or `Task<T>` | | |
| | `CS0029`: Cannot convert `Task<T>` to `T` | Add `await` before the call | | |
| | `CS0127`: Method returns `Task` but body returns value | Change return type to `Task<T>` | | |
| | `CS1983`: Return type of async method must be void, Task, Task<T>, ValueTask, or ValueTask<T> | Wrap the return type: `async string Foo()` → `async Task<string> Foo()` | | |
| | `CS1998`: Async method lacks `await` | Remove `async` if no awaits are needed, or the method is genuinely sync | | |
| | `CS0535`: Does not implement interface member | Update the interface to match the new async signatures (or vice versa) | | |
| | `CS7036`: No argument given for required parameter `cancellationToken` | Add `ct` argument at call sites after adding `CancellationToken` to signatures | | |
| | `CS1503`: Argument type mismatch (`Task<T>` passed where `T` expected) | Add `await` at the call site | |
if you want to add more?
Copilot
AI
Feb 25, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same issue as Step 1: grep without -P won't treat \b as a word boundary, so this command may not reliably find .Result usages. Adjust the command to use a regex flavor that supports word boundaries (or use \</\>), so the "should return zero results" guidance is accurate.
| grep -rn "\.Result\b\|\.Wait()\|\.GetAwaiter()\.GetResult()" --include="*.cs" . | |
| grep -rnP '\.Result\b|\.Wait\(\)|\.GetAwaiter\(\)\.GetResult\(\)' --include="*.cs" . |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is missing from all the examples. maybe indicate those are app code?
It might be worth mentioning as instructions, not just in anti-patterns -- if code is library add .ConfigureAwait(false) to every await. It should be used consistently or not at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this may be a big high level/conceptual for this skill?
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| <Project Sdk="Microsoft.NET.Sdk"> | ||
|
|
||
| <PropertyGroup> | ||
| <OutputType>Library</OutputType> | ||
| <TargetFramework>net8.0</TargetFramework> | ||
| <Nullable>enable</Nullable> | ||
| <ImplicitUsings>enable</ImplicitUsings> | ||
| </PropertyGroup> | ||
|
|
||
| <ItemGroup> | ||
| <PackageReference Include="Microsoft.Data.SqlClient" Version="5.2.0" /> | ||
| </ItemGroup> | ||
|
|
||
| </Project> |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,115 @@ | ||
| using Microsoft.Data.SqlClient; | ||
|
|
||
| namespace SyncService; | ||
|
|
||
| public interface IUserRepository | ||
| { | ||
| User GetById(int id); | ||
| List<User> GetAll(); | ||
| void Save(User user); | ||
| } | ||
|
|
||
| public class UserRepository : IUserRepository | ||
| { | ||
| private readonly string _connectionString; | ||
| private readonly HttpClient _httpClient; | ||
|
|
||
| public UserRepository(string connectionString, HttpClient httpClient) | ||
| { | ||
| _connectionString = connectionString; | ||
| _httpClient = httpClient; | ||
| } | ||
|
|
||
| public User GetById(int id) | ||
| { | ||
| using var connection = new SqlConnection(_connectionString); | ||
| connection.Open(); | ||
| using var command = new SqlCommand("SELECT Id, Name, Email FROM Users WHERE Id = @Id", connection); | ||
| command.Parameters.AddWithValue("@Id", id); | ||
| using var reader = command.ExecuteReader(); | ||
| if (reader.Read()) | ||
| { | ||
| return new User | ||
| { | ||
| Id = reader.GetInt32(0), | ||
| Name = reader.GetString(1), | ||
| Email = reader.GetString(2) | ||
| }; | ||
| } | ||
| throw new InvalidOperationException($"User {id} not found"); | ||
| } | ||
|
|
||
| public List<User> GetAll() | ||
| { | ||
| using var connection = new SqlConnection(_connectionString); | ||
| connection.Open(); | ||
| using var command = new SqlCommand("SELECT Id, Name, Email FROM Users", connection); | ||
| using var reader = command.ExecuteReader(); | ||
| var users = new List<User>(); | ||
| while (reader.Read()) | ||
| { | ||
| users.Add(new User | ||
| { | ||
| Id = reader.GetInt32(0), | ||
| Name = reader.GetString(1), | ||
| Email = reader.GetString(2) | ||
| }); | ||
| } | ||
| return users; | ||
| } | ||
|
|
||
| public void Save(User user) | ||
| { | ||
| using var connection = new SqlConnection(_connectionString); | ||
| connection.Open(); | ||
| using var command = new SqlCommand( | ||
| "INSERT INTO Users (Name, Email) VALUES (@Name, @Email)", connection); | ||
| command.Parameters.AddWithValue("@Name", user.Name); | ||
| command.Parameters.AddWithValue("@Email", user.Email); | ||
| command.ExecuteNonQuery(); | ||
| } | ||
| } | ||
|
|
||
| public class UserService | ||
| { | ||
| private readonly IUserRepository _repo; | ||
| private readonly HttpClient _httpClient; | ||
|
|
||
| public UserService(IUserRepository repo, HttpClient httpClient) | ||
| { | ||
| _repo = repo; | ||
| _httpClient = httpClient; | ||
| } | ||
|
|
||
| public User GetUserProfile(int userId) | ||
| { | ||
| var user = _repo.GetById(userId); | ||
|
|
||
| // Sync-over-async: blocking call | ||
| var response = _httpClient.Send(new HttpRequestMessage(HttpMethod.Get, $"/api/avatars/{userId}")); | ||
| var avatarUrl = new StreamReader(response.Content.ReadAsStream()).ReadToEnd(); | ||
| user.AvatarUrl = avatarUrl; | ||
|
|
||
| return user; | ||
| } | ||
|
|
||
| public List<User> GetAllUsers() | ||
| { | ||
| var users = _repo.GetAll(); | ||
| foreach (var user in users) | ||
| { | ||
| // Blocking call inside a loop | ||
| var task = _httpClient.GetStringAsync($"/api/avatars/{user.Id}"); | ||
| user.AvatarUrl = task.Result; // Anti-pattern: .Result blocks the thread | ||
| } | ||
| return users; | ||
| } | ||
| } | ||
|
|
||
| public class User | ||
| { | ||
| public int Id { get; set; } | ||
| public string Name { get; set; } = ""; | ||
| public string Email { get; set; } = ""; | ||
| public string? AvatarUrl { get; set; } | ||
| } |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,33 @@ | ||||||
| scenarios: | ||||||
| - name: "Refactor synchronous service to async" | ||||||
| prompt: "I have a service class with several synchronous database and HTTP calls. It's causing thread pool starvation under load. Can you convert it to async/await?" | ||||||
| setup: | ||||||
| copy_test_files: true | ||||||
| assertions: | ||||||
| - type: "output_matches" | ||||||
| pattern: "(async|await|Task<)" | ||||||
| - type: "output_matches" | ||||||
| pattern: "(CancellationToken|cancellation)" | ||||||
| - type: "output_not_matches" | ||||||
| pattern: "\\.Result\\b|\\.Wait\\(\\)" | ||||||
|
||||||
| pattern: "\\.Result\\b|\\.Wait\\(\\)" | |
| pattern: "\\.Result\\b\\s*;|\\.Wait\\(\\)\\s*;" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this really something the user would write? seems like a gimme as written. more likely the user would say "make DoIt() async so it's faster" and the AI would have to figure out that it's CPU bound?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you expect ConfigureAwait(false) in this UserService case? either way, should be a test for the opposite case (eg winforms) and verify in each case it's present or not as expected
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
move any part of use/not use into decription similar to
https://github.com/dotnet/runtime/pull/125005/changes#diff-ded9450821c1df27638def6250c00784c7f795e3a9c56ad13d09a34853a0d09bR3-R4
if it can avoid unnecessaryt reloading.
Possibly not all can go there. For example: user wants to parallelize work is something that can be known before loading the skill, and avoid load. Whereas possibly "code is CPU bound" might need loading the skill? but in general I think ?all of this should move up there.