Replace ATP pattern with async/await in SmtpClient (part 1) - #115366
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR modernizes the asynchronous programming model in SmtpClient and its related classes by replacing the legacy ATP (Begin/End) pattern with async/await. It also refactors test infrastructure to improve runtime performance and updates various Mime- and SMTP‑related components for a cleaner, task-based async workflow.
- Replaces legacy async patterns with generic async methods using IReadWriteAdapter.
- Refactors stream flushing and writing for simpler, synchronous and asynchronous paths.
- Improves test reliability by incorporating dual socket handling and timeout enforcement.
Reviewed Changes
Copilot reviewed 23 out of 24 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| MailMessageTest.cs | Updates the send routine to use SendAsync with a dynamically determined adapter type. |
| LoopbackSmtpServer.cs | Refactors socket binding to support dual-stack (IPv6/IPv4) based on socket address family. |
| LoopbackServerTestBase.cs | Applies WaitAsync for improved test-case timeouts. |
| LoggingTest.cs | Adjusts test invocation to explicitly pass a non-null argument. |
| MimeWriter.cs, MimePart.cs, MimeMultiPart.cs, MimeBasePart.cs | Eliminates ATP methods and replaces them with async/await and generic adapter calls. |
| BaseWriter.cs | Simplifies flushing logic by removing multi-result support in favor of direct stream writes. |
| SmtpTransport.cs, SmtpReplyReaderFactory.cs, SmtpReplyReader.cs, SmtpConnection.cs | Migrates legacy async IAsyncResult-based methods to task-based async methods. |
| SmtpClient.cs, MailMessage.cs | Updates send methods to leverage SendAsync and adapt Begin/End flows into task-based calls. |
| ReadWriteAdapter.cs | Introduces IReadWriteAdapter with implementations for both async and sync operations. |
| MailPriority.cs, MailWriter.cs, BufferBuilder.cs | Applies minor adjustments in error handling and stream management in line with the async model. |
Files not reviewed (1)
- src/libraries/System.Net.Mail/src/System.Net.Mail.csproj: Language not supported
Comments suppressed due to low confidence (2)
src/libraries/System.Net.Mail/tests/Functional/MailMessageTest.cs:264
- Consider adding a null check for 'syncSendAdapterType' before passing it to MakeGenericMethod to prevent a potential NullReferenceException if the type is not found.
var syncSendAdapterType = typeof(MailMessage).Assembly.GetTypes().FirstOrDefault(t => t.Name == "SyncReadWriteAdapter");
src/libraries/System.Net.Mail/src/System/Net/Mail/MailPriority.cs:144
- [nitpick] Remove the redundant semicolon after the empty catch block to improve code clarity.
catch (ArgumentException) { };
|
Tagging subscribers to this area: @dotnet/ncl |
wfurt
left a comment
wfurt
left a comment
There was a problem hiding this comment.
LGTM. I know this is large change but it is step right direction.
rokonec
left a comment
rokonec
left a comment
There was a problem hiding this comment.
I have noticed few internal methods without any usage. Please consider to clean it, if you do not have plan for future use of it.
Excellent job.
| public static Task WaitAsync(TaskCompletionSource<bool> waiter) | ||
| { | ||
| waiter.Task.GetAwaiter().GetResult(); | ||
| return Task.CompletedTask; |
There was a problem hiding this comment.
The idea of the SyncReadWriteAdapter is that it always returns a completed task, so that calling code can be fully synchronous (and can assert that task.IsCompleted). This file was copied from SslStream implementation, where it is used to deduplicate sync/async logic (in fact, it was the inspiration for this refactoring).
Followup PR removes this copy of the ReadWriteAdapters and shares the same file.
This PR replaces some of the ATP pattern (begin + end methods) with async/await equivalent. It also utilizes the IReadWriteAdapter trick that is used in SslStream to deduplicate sync and async code paths.
This PR focuses on the easy picks where the pattern can be clearly replaced, Following PR will focus on refactoring SmtpClient, SmtpConnection and SmtpTransport classes themselves, where the ATP pattern usage is more complicated.
Lastly, this PR improves test infrastructure by listening on dual sockets (if supported), this greatly speeds up the test suite on Windows (4s instead of 3 minutes, due to 2s timeout per test case because the test code attempted IPv6 first)