From: Marco Rossignoli Date: Wed, 26 Feb 2020 14:23:22 +0000 (+0100) Subject: Send QUIT on SmtpClient.Dispose() (#683) X-Git-Tag: submit/tizen/20210909.063632~9486 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=23e388ba40a3438fc50361583d7f8904d7bfa96d;p=platform%2Fupstream%2Fdotnet%2Fruntime.git 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 --- 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 --- 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 --- 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 --- 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 --- 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"); + } } }