Send QUIT on SmtpClient.Dispose() (#683)

* send QUIT on Dispose()

* add try/finally

* simplify null check

* address PR feedback

* send quit in non-blocking mode

* resolve conflicts

diff --git a/src/libraries/System.Net.Mail/src/System/Net/Mail/SmtpClient.cs b/src/libraries/System.Net.Mail/src/System/Net/Mail/SmtpClient.cs
index 255d710..5ae9dc9 100644 (file)
--- a/src/libraries/System.Net.Mail/src/System/Net/Mail/SmtpClient.cs
+++ b/src/libraries/System.Net.Mail/src/System/Net/Mail/SmtpClient.cs
@@ -543,7 +543,6 @@ namespace System.Net.Mail
                     _message = message;
                     message.Send(writer, DeliveryMethod != SmtpDeliveryMethod.Network, allowUnicode);
                     writer.Close();
-                    _transport.ReleaseConnection();
 
                     //throw if we couldn't send to any of the recipients
                     if (DeliveryMethod == SmtpDeliveryMethod.Network && recipientException != null)
@@ -690,7 +689,6 @@ namespace System.Net.Mail
                                 if (_writer != null)
                                     _writer.Close();
 
-                                _transport.ReleaseConnection();
                                 AsyncCompletedEventArgs eventArgs = new AsyncCompletedEventArgs(null, false, _asyncOp.UserSuppliedState);
                                 InCall = false;
                                 _asyncOp.PostOperationCompleted(_onSendCompletedDelegate, eventArgs);
@@ -937,7 +935,6 @@ namespace System.Net.Mail
                             exception = se;
                         }
                     }
-                    _transport.ReleaseConnection();
                 }
             }
             finally
@@ -1095,17 +1092,11 @@ namespace System.Net.Mail
                     _cancelled = true;
                     Abort();
                 }
-
-                if ((_transport != null))
-                {
-                    _transport.ReleaseConnection();
-                }
-
-                if (_timer != null)
+                else
                 {
-                    _timer.Dispose();
+                    _transport?.ReleaseConnection();
                 }
-
+                _timer?.Dispose();
                 _disposed = true;
             }
         }

diff --git a/src/libraries/System.Net.Mail/src/System/Net/Mail/SmtpCommands.cs b/src/libraries/System.Net.Mail/src/System/Net/Mail/SmtpCommands.cs
index b6d1efd..36fd589 100644 (file)
--- a/src/libraries/System.Net.Mail/src/System/Net/Mail/SmtpCommands.cs
+++ b/src/libraries/System.Net.Mail/src/System/Net/Mail/SmtpCommands.cs
@@ -641,6 +641,29 @@ namespace System.Net.Mail
         }
     }
 
+    internal static class QuitCommand
+    {
+        private static void PrepareCommand(SmtpConnection conn)
+        {
+            if (conn.IsStreamOpen)
+            {
+                throw new InvalidOperationException(SR.SmtpDataStreamOpen);
+            }
+
+            conn.BufferBuilder.Append(SmtpCommands.Quit);
+        }
+
+        internal static void Send(SmtpConnection conn)
+        {
+            PrepareCommand(conn);
+
+            // We simply flush and don't read the response
+            // to avoid blocking call that will impact users
+            // that are using async api, since this code
+            // will run on Dispose()
+            conn.Flush();
+        }
+    }
 
     internal static class SmtpCommands
     {

diff --git a/src/libraries/System.Net.Mail/src/System/Net/Mail/SmtpConnection.cs b/src/libraries/System.Net.Mail/src/System/Net/Mail/SmtpConnection.cs
index e8be037..12852ea 100644 (file)
--- a/src/libraries/System.Net.Mail/src/System/Net/Mail/SmtpConnection.cs
+++ b/src/libraries/System.Net.Mail/src/System/Net/Mail/SmtpConnection.cs
@@ -124,7 +124,7 @@ namespace System.Net.Mail
             _bufferBuilder.Reset();
         }
 
-        internal void ReleaseConnection()
+        private void ShutdownConnection(bool isAbort)
         {
             if (!_isClosed)
             {
@@ -132,14 +132,42 @@ namespace System.Net.Mail
                 {
                     if (!_isClosed && _tcpClient != null)
                     {
-                        //free cbt buffer
-                        if (_channelBindingToken != null)
+                        try
+                        {
+                            try
+                            {
+                                if (isAbort)
+                                {
+                                    // Must destroy manually since sending a QUIT here might not be
+                                    // interpreted correctly by the server if it's in the middle of a
+                                    // DATA command or some similar situation.  This may send a RST
+                                    // but this is ok in this situation.  Do not reuse this connection
+                                    _tcpClient.LingerState = new LingerOption(true, 0);
+                                }
+                                else
+                                {
+                                    // Gracefully close the transmission channel
+                                    _tcpClient.Client.Blocking = false;
+                                    QuitCommand.Send(this);
+                                }
+                            }
+                            finally
+                            {
+                                //free cbt buffer
+                                _channelBindingToken?.Close();
+                                _networkStream?.Close();
+                                _tcpClient.Dispose();
+                            }
+                        }
+                        catch (IOException)
                         {
-                            _channelBindingToken.Close();
+                            // Network failure
+                        }
+                        catch (ObjectDisposedException)
+                        {
+                            // See https://github.com/dotnet/corefx/issues/40711, and potentially
+                            // catch additional exception types here if need demonstrates.
                         }
-
-                        _networkStream?.Close();
-                        _tcpClient.Dispose();
                     }
 
                     _isClosed = true;
@@ -149,36 +177,14 @@ namespace System.Net.Mail
             _isConnected = false;
         }
 
+        internal void ReleaseConnection()
+        {
+            ShutdownConnection(false);
+        }
+
         internal void Abort()
         {
-            if (!_isClosed)
-            {
-                try
-                {
-                    lock (this)
-                    {
-                        if (!_isClosed && _tcpClient != null)
-                        {
-                            _channelBindingToken?.Close();
-
-                            // Must destroy manually since sending a QUIT here might not be
-                            // interpreted correctly by the server if it's in the middle of a
-                            // DATA command or some similar situation.  This may send a RST
-                            // but this is ok in this situation.  Do not reuse this connection
-                            _tcpClient.LingerState = new LingerOption(true, 0);
-                            _networkStream?.Close();
-                            _tcpClient.Dispose();
-                        }
-                        _isClosed = true;
-                    }
-                }
-                catch (ObjectDisposedException)
-                {
-                    // See https://github.com/dotnet/runtime/issues/30732, and potentially
-                    // catch additional exception types here if need demonstrates.
-                }
-            }
-            _isConnected = false;
+            ShutdownConnection(true);
         }
 
         internal void GetConnection(string host, int port)

diff --git a/src/libraries/System.Net.Mail/src/System/Net/Mail/SmtpTransport.cs b/src/libraries/System.Net.Mail/src/System/Net/Mail/SmtpTransport.cs
index 4a55dff..281a00c 100644 (file)
--- a/src/libraries/System.Net.Mail/src/System/Net/Mail/SmtpTransport.cs
+++ b/src/libraries/System.Net.Mail/src/System/Net/Mail/SmtpTransport.cs
@@ -195,10 +195,7 @@ namespace System.Net.Mail
 
         internal void ReleaseConnection()
         {
-            if (_connection != null)
-            {
-                _connection.ReleaseConnection();
-            }
+            _connection?.ReleaseConnection();
         }
 
         internal void Abort()

diff --git a/src/libraries/System.Net.Mail/tests/Functional/SmtpClientTest.cs b/src/libraries/System.Net.Mail/tests/Functional/SmtpClientTest.cs
index 7f1e3f7..7e1c86d 100644 (file)
--- a/src/libraries/System.Net.Mail/tests/Functional/SmtpClientTest.cs
+++ b/src/libraries/System.Net.Mail/tests/Functional/SmtpClientTest.cs
@@ -488,5 +488,39 @@ namespace System.Net.Mail.Tests
         }
 
         private static string GetClientDomain() => IPGlobalProperties.GetIPGlobalProperties().HostName.Trim().ToLower();
+
+        [Theory]
+        [InlineData(false)]
+        [InlineData(true)]
+        public async Task SendMail_SendQUITOnDispose(bool asyncSend)
+        {
+            bool quitMessageReceived = false;
+            using ManualResetEventSlim quitReceived = new ManualResetEventSlim();
+            using var server = new LoopbackSmtpServer();
+            server.OnQuitReceived += _ =>
+            {
+                quitMessageReceived = true;
+                quitReceived.Set();
+            };
+
+            using (SmtpClient client = server.CreateClient())
+            {
+                client.Credentials = new NetworkCredential("Foo", "Bar");
+                MailMessage msg = new MailMessage("foo@example.com", "bar@example.com", "hello", "howdydoo");
+                if (asyncSend)
+                {
+                    await client.SendMailAsync(msg).TimeoutAfter((int)TimeSpan.FromSeconds(30).TotalMilliseconds);
+                }
+                else
+                {
+                    client.Send(msg);
+                }
+                Assert.False(quitMessageReceived, "QUIT received");
+            }
+
+            // There is a latency between send/receive.
+            quitReceived.Wait(TimeSpan.FromSeconds(30));
+            Assert.True(quitMessageReceived, "QUIT message not received");
+        }
     }
 }