Replace ATP pattern with async/await in SmtpClient (part 2) #115722
Merged
Conversation
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
There was a problem hiding this comment.
Pull Request Overview
This PR replaces the ATP pattern with first‑class async/await implementations for SmtpClient and related components, streamlining asynchronous mail sending and connection handling.
- Converts legacy Begin/End method patterns to async/await implementations.
- Updates tests and project file references to use the common ReadWriteAdapter implementation.
- Removes redundant legacy code in System.Net.Mail and System.Net.Security.
Reviewed Changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| System.Net.Security.Unit.Tests.csproj | Removed legacy ReadWriteAdapter reference and added the Common version. |
| System.Net.Security (ReadWriteAdapter.cs) | Removed the legacy ReadWriteAdapter implementation. |
| System.Net.Mail.Unit.Tests.csproj | Updated test project references to use the Common ReadWriteAdapter. |
| SmtpClientTest.cs | Added a test to validate SendAsync cancellation behavior. |
| SmtpTransport.cs | Refactored connection and mail sending methods to async/await and removed legacy synchronous methods. |
| SmtpReplyReaderFactory.cs / SmtpReplyReader.cs | Removed synchronous wrappers in favor of fully asynchronous implementations. |
| SmtpCommands.cs | Eliminated obsolete command wrappers, relying solely on async patterns. |
| MailMessage.cs | Removed synchronous send wrappers in favor of async implementations. |
| System.Net.Mail.csproj | Updated file references and removed legacy items. |
| Common/ReadWriteAdapter.cs | Moved the ReadWriteAdapter into the shared Common folder with an updated namespace. |
Comments suppressed due to low confidence (1)
src/libraries/System.Net/Mail/SmtpTransport.cs:151
- [nitpick] Consider using a dedicated private lock object instead of locking on 'this' to improve encapsulation and reduce potential deadlock risks.
lock (this)
|
Tagging subscribers to this area: @dotnet/ncl |
|
Nice diff! |
This was referenced May 19, 2025
Open
rokonec
approved these changes
May 23, 2025
src/libraries/System.Net.Mail/src/System/Net/Mail/SmtpClient.cs
Outdated
Show resolved
Hide resolved
| CancellationTokenSource currentCts = Interlocked.Exchange(ref _pendingSendCts, new CancellationTokenSource()); | ||
|
|
||
| currentCts.Cancel(); | ||
| currentCts.Dispose(); |
There was a problem hiding this comment.
Is it OK to not call Abort() here? It was called in previous implementation.
There was a problem hiding this comment.
The abort is called by the SendAsyncInternal when processing any fatal exception.
src/libraries/System.Net.Mail/src/System/Net/Mail/SmtpClient.cs
Outdated
Show resolved
Hide resolved
MihaZupan
approved these changes
May 26, 2025
src/libraries/System.Net.Mail/src/System/Net/Mail/SmtpConnection.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Mail/src/System/Net/Mail/SmtpClient.cs
Outdated
Show resolved
Hide resolved
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.
This PR removes the rest of the ATP pattern usage in System.Net.Mail.
Formerly, the newer
SendMailAsyncmethods were implemented as a wrapper over (void returning)SendAsyncand listening for completion viaSendCompletedasync event. This PR reverses that order, so thatSendMailAsyncis implemented as first class method, andSendAsyncis implemented as a wrapper overSendMailAsyncfor compatibility.The exception handling is a bit complicated to preserve outward behavior with regards to wrapping the exception in SmtpException.
Fixes #115070