Invalidate SmtpClient cached connection when connection-affecting properties change

[rzikm]

Why

SmtpClient caches a live SmtpConnection in its transport and reuses it across Send calls. However, the connection-affecting property setters never dropped the cached connection. As a result, changing configuration between sends kept delivering mail over the original connection instead of re-establishing one for the new configuration. The most visible case was Host/Port (mail kept going to the original server), but the same stale-reuse problem applied to every property that determines how the connection is established.

What

When a connection-affecting property actually changes, the cached transport connection is invalidated so the next send re-establishes a fresh connection for the new configuration. This now covers:

• Host and Port
• Credentials / UseDefaultCredentials (SMTP AUTH is per-connection)
• EnableSsl
• TargetName (the SPN used for authentication)

Changing Host also updates the default TargetName (SMTPSVC/<host>) when the target name was not set explicitly, so authentication targets the new host.

Rather than doing potentially blocking work in a property setter, the setters simply mark the transport as stale (a non-blocking flag). The stale connection is aborted and replaced lazily on the next send inside GetConnectionAsync. Aborting (rather than a graceful QUIT) avoids synchronous network I/O on the async send path. Normal connection reuse is preserved when a value is unchanged, and the setters throw InvalidOperationException (SmtpInvalidOperationDuringSend) if changed while a send is in progress, so invalidation never races an active send.

Tests

Added regression tests in SmtpClientConnectionTest and SmtpClientTlsTest, running across all three send paths (Send, SendAsync, SendMailAsync):

• ChangingConnectionProperty_EstablishesNewConnection - theory over Host, Credentials, and TargetName; asserts a new connection is established and, for the Host case, that the default TargetName follows the new host.
• ChangingHost_PreservesExplicitlySetTargetName - an explicitly set TargetName is not overwritten when Host changes.
• ChangingPort_DoesNotReuseConnectionToPreviousServer - points the client at a second loopback server and asserts the next message reaches the new server, not the original.
• EnableSsl_ChangedAfterConnect_EstablishesNewEncryptedConnection - toggling EnableSsl after a plaintext connection forces a new encrypted connection.

The targeted SmtpClientConnectionTest and SmtpClientTlsTest classes (60 tests across the three send modes) pass; the connection-property tests were confirmed to fail without the fix and pass with it.

Note

This PR was authored by GitHub Copilot.

[azure-pipelines]

Azure Pipelines:
Successfully started running 3 pipeline(s).
13 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[dotnet-policy-service]

Tagging subscribers to this area: @karelz, @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

[Copilot]

Pull request overview

This PR updates System.Net.Mail.SmtpClient so that changing Host or Port invalidates any cached SMTP transport connection, ensuring subsequent sends connect to the newly configured endpoint rather than reusing a connection established for the previous host/port.

Changes:

• Release the cached transport connection when SmtpClient.Host changes.
• Release the cached transport connection when SmtpClient.Port changes.
• Add functional regression tests to verify host/port changes result in new connections and correct delivery.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
src/libraries/System.Net.Mail/src/System/Net/Mail/SmtpClient.cs	Releases the cached SMTP transport connection when Host/Port changes.
src/libraries/System.Net.Mail/tests/Functional/SmtpClientConnectionTest.cs	Adds regression tests ensuring host/port changes establish a new connection and deliver to the intended server.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

src/libraries/System.Net.Mail/src/System/Net/Mail/SmtpTransport.cs:23

• _stale is written from property setters (no lock) and read from IsConnected on the send path; without synchronization this can be lost/seen late across threads, causing an old connection to be reused even after an invalidating configuration change. Marking the field volatile makes the invalidation reliably observable without widening locking.

        private readonly SmtpClient _client;
        private ICredentialsByHost? _credentials;
        private bool _shouldAbort;
        private bool _stale;

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

src/libraries/System.Net.Mail/src/System/Net/Mail/SmtpTransport.cs:105

• GetConnectionAsync performs potentially blocking shutdown work (ReleaseConnection() / Abort()) while holding the SmtpTransport lock. This increases lock hold times across network I/O (QUIT) and can delay SmtpTransport.Abort() (used by the send timeout path). Consider clearing _connection under the lock, releasing/aborting the previous connection outside the lock, then reacquiring the lock to create the new connection (so Abort() during shutdown can still flow through _shouldAbort).

        {
            lock (this)
            {
                // Release any previously cached connection (for example one that became stale
                // after a configuration change, or one whose connect attempt failed) before

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

src/libraries/System.Net.Mail/tests/Functional/SmtpClientConnectionTest.cs:16

• ConnectionAffectingProperty is only used as test data within this file; making it public unnecessarily expands the surface area of the test assembly. Consider making it internal (or nesting it in the test type) since nothing outside needs it.

    public enum ConnectionAffectingProperty
    {
        Host,
        Credentials,
        TargetName,

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

src/libraries/System.Net.Mail/src/System/Net/Mail/SmtpClient.cs:370

• This comment says TargetName participates in "TLS", but the TLS handshake uses the host parameter as TargetHost; TargetName is used for authentication/SPN. Update the comment to avoid implying TLS behavior.

                    // The target name participates in connection establishment (authentication
                    // and TLS), so invalidate any cached connection to force a new one on the
                    // next send.

src/libraries/System.Net.Mail/tests/Functional/SmtpClientConnectionTest.cs:12

• This enum is only used within this test file; declaring it as public unnecessarily expands the test assembly's public surface area. Make it non-public (default/internal) to keep scope narrower.

    public enum ConnectionAffectingProperty

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

src/libraries/System.Net.Mail/src/System/Net/Mail/SmtpTransport.cs:129

• GetConnectionAsync<TIOAdapter> may synchronously perform a graceful QUIT (ReleaseConnection) on the previously cached connection before establishing the new one. Since this runs on the SendMailAsync path before the first await, it can block the caller thread (e.g., UI thread) and also can’t be interrupted by Abort() while it’s in the QUIT/close path (because _connection is already cleared). For stale/failed connection invalidation, aborting the old connection is sufficient and avoids synchronous network I/O in an async code path.

                if (previousConnection.IsConnected)
                {
                    previousConnection.ReleaseConnection();
                }

src/libraries/System.Net.Mail/src/System/Net/Mail/SmtpTransport.cs:195

• The PR title/description focus on invalidating the cached connection when Host/Port change, but the implementation also invalidates on Credentials, EnableSsl, and TargetName changes (as described in this comment). Please update the PR description/title to reflect the broader behavior change so reviewers and backporters don’t miss it.

        // Marks any cached connection as stale without performing blocking work. The connection
        // is gracefully released and replaced the next time one is established (see
        // GetConnectionAsync). This is called when a property that affects how the connection is
        // established (host, port, credentials, SSL settings, target name) changes.
        internal void InvalidateCachedConnection()

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

src/libraries/System.Net.Mail/tests/Functional/SmtpClientConnectionTest.cs:12

• The ConnectionAffectingProperty enum is only used within this test file and doesn’t need to be public. Keeping it public unnecessarily expands the test assembly’s surface area (and makes it easier to accidentally depend on from other tests).

    public enum ConnectionAffectingProperty

[rzikm]

/ba-g Test failures are unrelated, no SMTP related failures