Analysis for async performance issues #4128
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| --- | ||
| applyTo: "**/*.cs" | ||
| --- | ||
| # C# Coding Style | ||
|
|
||
| All C# code must follow the rules in `policy/coding-style.md`. Read that file | ||
| before writing or modifying any C# code in this repository. |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,30 @@ | ||
| --- | ||
| applyTo: "plans/**/*.md" | ||
| --- | ||
| # Formatting Rules for Plan Documents | ||
|
|
||
| All markdown files under `plans/` must follow the rules in `plans/FORMATTING.md` and pass | ||
| `markdownlint-cli2` with the config at `plans/.markdownlint.jsonc`. | ||
|
|
||
| ## Mandatory Workflow | ||
|
|
||
| 1. Read `plans/FORMATTING.md` before creating or editing any markdown file under `plans/`. | ||
| 2. After making edits, run the linter and fix all errors before finishing: | ||
|
|
||
| ```bash | ||
| markdownlint-cli2 "plans/**/*.md" | ||
| ``` | ||
|
|
||
| ## Key Rules | ||
|
|
||
| - **Line width: 100 characters** — not 80. Fill lines close to 100 before wrapping. | ||
| - Blank lines required around: headings (MD022), lists (MD032), fenced code blocks (MD031). | ||
| - Fenced code blocks must have a language specified (MD040) — e.g., `csharp`, `text`, `bash`. | ||
| - Ordered lists use sequential numbers (1, 2, 3), not all `1.` (MD029). | ||
| - No trailing punctuation (`:` or `.`) in headings (MD026). | ||
| - No duplicate sibling headings — make each unique (MD024). | ||
| - Table pipes must have consistent spacing (MD060). | ||
| - Markdown links `[text](url)` are indivisible — never split across lines. | ||
| - Do not let issue references (e.g., `#3356`) start a line — this triggers MD018 (no-missing-space-atx). | ||
| - Metadata lines (`**Priority:** High`) are standalone key-value pairs — do not merge into paragraphs. | ||
| - UTF-8 encoding, LF line endings, single trailing newline. |
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,22 @@ | ||
| // Markdownlint config for plans/ directory. | ||
| // Equivalent to the former .mdl-style.rb rules. | ||
| { | ||
| // All rules enabled by default (markdownlint-cli2 default). | ||
|
|
||
| // MD007: Unordered list indentation — 2 spaces per level. | ||
| "MD007": { "indent": 2 }, | ||
|
|
||
| // MD013: Line length — max 100 chars, skip code blocks and tables. | ||
| "MD013": { | ||
| "line_length": 100, | ||
| "code_blocks": false, | ||
| "tables": false | ||
| }, | ||
|
|
||
| // MD029: Ordered list style — sequential numbering (1, 2, 3). | ||
| "MD029": { "style": "ordered" }, | ||
|
|
||
| // MD033: Inline HTML — allow all elements (needed for generic type | ||
| // syntax like IAsyncEnumerable<T> in headings and text). | ||
| "MD033": false | ||
| } |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,93 @@ | ||
| # Formatting Rules for Plan Documents | ||
|
|
||
| All markdown files under `plans/` must follow these rules and pass `markdownlint-cli2` with the | ||
| config in `plans/.markdownlint.jsonc`. | ||
|
|
||
| ## Linting | ||
|
|
||
| ### Installing markdownlint-cli2 | ||
|
|
||
| `markdownlint-cli2` is a Node.js package. Install it with: | ||
|
|
||
| ```bash | ||
| sudo apt update && sudo apt install -y nodejs npm | ||
| sudo npm install -g markdownlint-cli2 | ||
| ``` | ||
|
|
||
| If you already have `npm`, skip the `apt` step. | ||
|
|
||
| ### Running markdownlint-cli2 | ||
|
|
||
| Run the linter before committing: | ||
|
|
||
| ```bash | ||
| markdownlint-cli2 "plans/async-performance/**/*.md" | ||
| ``` | ||
|
|
||
| The VS Code **markdownlint** extension uses the same engine and config file automatically. | ||
|
|
||
| The config sets: | ||
|
|
||
| - **MD007** unordered list indent to 2 spaces per level | ||
| - **MD013** line length to 100 (code blocks and tables exempt) | ||
| - **MD029** ordered list style to `:ordered` (sequential: 1, 2, 3) | ||
| - **MD033** disabled (inline HTML allowed for generic type syntax like `IAsyncEnumerable<T>` in | ||
| headings) | ||
|
|
||
| ## Line Length | ||
|
|
||
| - **Max line width: 100 characters** | ||
| - Wrap paragraph text and list items at 100 chars | ||
| - Do NOT wrap these elements (even if over 100 chars): | ||
| - Table rows | ||
| - Headings | ||
| - Fenced code blocks (``` or ~~~) | ||
| - Lines containing a single indivisible token (e.g., a backtick-wrapped file path or a markdown | ||
| link) | ||
| - Horizontal rules | ||
|
|
||
| ## Encoding | ||
|
|
||
| - **UTF-8**, no BOM | ||
| - **LF** line endings (no CR or CRLF) | ||
| - Files must end with exactly one trailing newline | ||
|
|
||
| ## Markdown Structure | ||
|
|
||
| These rules correspond to markdownlint rules that are enforced: | ||
|
|
||
| - **Blank lines around headings** (MD022) — always put an empty line before and after `#` headings | ||
| - **Blank lines around lists** (MD032) — always put an empty line before the first list item and | ||
| after the last | ||
| - **Blank lines around fenced code blocks** (MD031) — always put an empty line before ``` and after | ||
| the closing ``` | ||
| - **Ordered list numbering** (MD029) — use sequential numbers (1, 2, 3), not all `1.` | ||
| - **No trailing punctuation in headings** (MD026) — don't end headings with `:` or `.` | ||
| - **ATX headings only** (MD003) — use `#` style, not underline (setext) style | ||
| - **No duplicate sibling headings** (MD024) — differentiate repeated heading text (e.g., | ||
| "Recommendation" under each priority should be unique like "P1 Recommendation") | ||
| - **No multiple consecutive blank lines** — use a single blank line as separator | ||
|
|
||
| ## Wrapping Details | ||
|
|
||
| - List continuation lines align to the content start of the list marker (e.g., 2 spaces past `-` or | ||
| `1.`) | ||
| - Preserve existing indentation levels | ||
| - Do not break words or hyphenated terms across lines | ||
| - Markdown links `[text](url)` are indivisible — never split a link across two lines | ||
|
|
||
| ## AI Agent Instructions | ||
|
|
||
| When generating or editing markdown files under `plans/`, fill lines to the **100-character | ||
| maximum**. A common mistake is wrapping at ~80 characters (a typical editor default). To avoid this: | ||
|
|
||
| 1. **Set your wrap target to exactly 100.** Do not use 80. The MD013 rule in | ||
| `.markdownlint.jsonc` enforces 100. | ||
| 2. **Fill each line as close to 100 as possible** before wrapping to the next line. If the next word | ||
| fits within 100 characters, it belongs on the current line. | ||
| 3. **Check your output.** If most paragraph lines end around column 75–85, you are wrapping too | ||
| short. Re-wrap to 100. | ||
| 4. **Treat markdown links as single tokens.** Never break `[display text](url)` across lines. If a | ||
| link won't fit on the current line, move the entire link to the next line. | ||
| 5. **Keep metadata lines separate.** Lines like `**Priority:** High` and `**Complexity:** Low` are | ||
| standalone key-value pairs — do not merge them into the preceding or following paragraph. |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,59 @@ | ||
| # Fix 1: Wire Up Factory Routing | ||
|
|
||
| **Priority:** Critical — pool V2 cannot be used at all without this | ||
| **Risk:** Low — guarded by AppContext switch | ||
|
|
||
| ## Problem | ||
|
|
||
| In `SqlConnectionFactory.cs`, the `UseConnectionPoolV2` code path throws `NotImplementedException` | ||
| instead of constructing a `ChannelDbConnectionPool`: | ||
|
|
||
| ```csharp | ||
| if (LocalAppContextSwitches.UseConnectionPoolV2) | ||
| { | ||
| throw new NotImplementedException(); // ← Must be replaced | ||
| } | ||
| else | ||
| { | ||
| newPool = new WaitHandleDbConnectionPool(...); | ||
| } | ||
| ``` | ||
|
|
||
| ## Location | ||
|
|
||
| **File:** `src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnectionFactory.cs` | ||
| **Method:** `DbConnectionPoolGroup.GetConnectionPool()` (around line 180) | ||
|
|
||
| ## Changes Required | ||
|
|
||
| 1. Replace `throw new NotImplementedException()` with: | ||
|
|
||
| ```csharp | ||
| newPool = new ChannelDbConnectionPool( | ||
| connectionPoolIdentity, | ||
| connectionPoolOptions, | ||
| connectionPoolProviderInfo, | ||
| owningObject, | ||
| this); | ||
| ``` | ||
|
|
||
| 2. Verify the `ChannelDbConnectionPool` constructor signature matches the parameters available at | ||
| this call site. The existing `IDbConnectionPool` interface defines the contract. | ||
|
|
||
| 3. Ensure `ChannelDbConnectionPool` is accessible from `SqlConnectionFactory` (same | ||
| namespace/assembly — it is). | ||
|
|
||
| ## Testing | ||
|
|
||
| - Unit test: Set `UseConnectionPoolV2 = true`, create a `SqlConnection`, verify pool construction | ||
| doesn't throw. | ||
|
|
||
| - This test will initially fail on `Startup()` (Fix 2), so it should be submitted together with Fix | ||
| 2. | ||
|
|
||
| ## Risk Assessment | ||
|
|
||
| - **Very low risk** — The `UseConnectionPoolV2` switch defaults to `false`. No production code is | ||
| affected unless the switch is explicitly enabled. | ||
|
|
||
| - Must verify that existing tests with `UseConnectionPoolV2 = false` still pass. |
Oops, something went wrong.
Oops, something went wrong.
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.
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.
PackageVersionentries forMicrosoft.Data.SqlClientandMicrosoft.Data.SqlClient.Extensions.Azurewere added unconditionally. This creates two concrete restore/build hazards:-p:ReferenceType=Package, the conditionalItemGroupbelow also defines versions for these same package IDs, resulting in duplicatePackageVersionitems.doc/Directory.Packages.propsimports the root props and also definesPackageVersionfor these package IDs, which will now be duplicates as well.To avoid breaking package-mode builds and doc builds, consider removing these unconditioned entries and instead scoping the versions to the
plans/app (e.g., add aDirectory.Packages.propsunderplans/async-performance/apps/ThreadStarvation/similar todoc/apps/AzureAuthentication/Directory.Packages.props, or usePackageVersion Update=...in the importing props where overriding is intended).