Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Send QUIT on SmtpClient.Dispose() #683

Merged
merged 6 commits into from
Feb 26, 2020
Merged

Send QUIT on SmtpClient.Dispose() #683

merged 6 commits into from
Feb 26, 2020

Conversation

MarcoRossignoli
Copy link
Member

@MarcoRossignoli MarcoRossignoli commented Dec 9, 2019 •
edited
Loading

Fixes https://github.com/dotnet/corefx/issues/34868

When code was ported SmtpConnection pooling present on old code base was removed but seem that semantic wasn't correctly updated.
On netfx code base the call to ReleaseConnection() ends to place where the connection is cached for future use(creation callback)
When we call SmtpClient.Dispose() we end calling pool manager cleanup that calls real dispose on pooled stream with the result of send QUIT message on all opened network stream

On core this mechanism is not present, so until today every ReleaseConnection() called after send messages closes socket/stream violating RFC https://tools.ietf.org/html/rfc5321#section-4.1.1.10

The sender MUST NOT intentionally close the transmission
channel until it sends a QUIT command, and it SHOULD wait until it
receives the reply (even if there was an error response to a previous
command).

If users forget to dispose network stream will be closed in a "bad" way(like today) by finalizer
I didn't found any finalizer on pool on netfx code base, btw it's a bit hard navigate without "Go to" vs feature(it doesn't work), so I could have missed some place where PooledStream are released thank's to some finalizer.

However the guide is clear Sends a QUIT message to the SMTP server, gracefully ends the TCP connection, and releases all resources used by the current instance of the SmtpClient class. so we should always call Dispose()(missing in the main example https://docs.microsoft.com/en-us/dotnet/api/system.net.mail.smtpclient?view=netframework-4.8#examples).

cc: @davidsh @wfurt

Bonus: during test I found that if we call 2 time one after another async send, test hang, I don't know if it's related to LoopbackSmtpServer, this issue it's not related to this PR the behaviour is already present.

@davidsh davidsh requested a review from a team December 9, 2019 14:15
@MihaZupan
Copy link
Member

Bonus: during test I found that if we call 2 time one after another async send, test hang, I don't know if it's related to LoopbackSmtpServer, this issue it's not related to this PR the behaviour is already present.

It is related to LoopbackSmtpServer. It will only listen for a single connection unless you set ReceiveMultipleConnections.

@MihaZupan
Copy link
Member

How does this handle the case when SmtpClient is in the middle of sending the Body when DIspose is called (as mentioned in https://github.com/dotnet/corefx/issues/34868#issuecomment-523087400)?

I assume this would corrupt the stream?

@MarcoRossignoli
Copy link
Member Author

MarcoRossignoli commented Dec 9, 2019 •
edited
Loading

I assume this would corrupt the stream?

InCall will be true and we end to Abort()

Same as netfx https://referencesource.microsoft.com/#System/net/System/Net/mail/SmtpClient.cs,967

@scalablecory
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 10 pipeline(s).

@davidsh
Copy link
Contributor

davidsh commented Dec 9, 2019 •
edited
Loading

See latest discussion on https://github.com/dotnet/corefx/issues/34868. We might end up adding a new API for SmtpClient instead of trying to force the 'QUIT' in Dispose().

@davidsh davidsh added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Dec 9, 2019
@davidsh
Copy link
Contributor

davidsh commented Dec 17, 2019

Based on latest discussion, I'm fine with the design approach in this PR for implementing QUIT in Dispose().

@davidsh davidsh removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Dec 17, 2019
@davidsh davidsh added this to the 5.0 milestone Dec 17, 2019
@MarcoRossignoli MarcoRossignoli mentioned this pull request Dec 17, 2019
Closed
@MarcoRossignoli
Copy link
Member Author

/azp list

@azure-pipelines
Copy link

Commenter does not have sufficient privileges for PR 683 in repo dotnet/runtime

@MarcoRossignoli
Copy link
Member Author

/azp list

@azure-pipelines
Copy link

Commenter does not have sufficient privileges for PR 683 in repo dotnet/runtime

@karelz karelz requested review from a team and scalablecory and removed request for davidsh January 16, 2020 19:59
scalablecory
scalablecory suggested changes Jan 16, 2020
View reviewed changes
Copy link
Contributor

@scalablecory scalablecory left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, small changes requested and then we'll find out why /azp isn't working for you.

src/libraries/System.Net.Mail/src/System/Net/Mail/SmtpCommands.cs Outdated Show resolved Hide resolved
src/libraries/System.Net.Mail/src/System/Net/Mail/SmtpCommands.cs Outdated Show resolved Hide resolved
src/libraries/System.Net.Mail/src/System/Net/Mail/SmtpCommands.cs Outdated Show resolved Hide resolved
src/libraries/System.Net.Mail/src/System/Net/Mail/SmtpConnection.cs Outdated Show resolved Hide resolved
@MarcoRossignoli
Copy link
Member Author

then we'll find out why /azp isn't working for you.

Would be great 😍

@scalablecory
Copy link
Contributor

/azp run runtime-libraries outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

MarcoRossignoli
MarcoRossignoli commented Feb 7, 2020
View reviewed changes
src/libraries/System.Net.Mail/src/System/Net/Mail/SmtpClient.cs
_transport?.ReleaseConnection();
else
{
_transport?.ReleaseConnection();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@scalablecory @tmds in this way the path should be clear, if we're incall we'll abort without quit otherwise cleanup with quit.

MarcoRossignoli
MarcoRossignoli commented Feb 7, 2020
View reviewed changes
src/libraries/System.Net.Mail/src/System/Net/Mail/SmtpConnection.cs
@@ -160,7 +160,7 @@ private void ShutdownConnection(bool isAbort)
}
}
catch (IOException) { }
catch (InvalidOperationException)
catch (ObjectDisposedException)
Copy link
Member Author

@MarcoRossignoli MarcoRossignoli Feb 7, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@scalablecory @tmat I added ObjectDisposedException because we throw InvalidOperationException from quit command and we could hide a bug.
IOException for possible network failure.

@MarcoRossignoli
Copy link
Member Author

/azp list

@azure-pipelines
Copy link

Commenter does not have sufficient privileges for PR 683 in repo dotnet/runtime

@davidsh
Copy link
Contributor

davidsh commented Feb 15, 2020

/azp list

@azure-pipelines
Copy link

CI/CD Pipelines for this repository:

@davidsh
Copy link
Contributor

davidsh commented Feb 15, 2020

/azp run runtime-libraries outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@davidsh
Copy link
Contributor

davidsh commented Feb 20, 2020

@MarcoRossignoli I think this PR is close to being merged. But there is a conflict now. Can you please re-base this PR against current 'master'?

@MarcoRossignoli
Copy link
Member Author

@MarcoRossignoli I think this PR is close to being merged. But there is a conflict now. Can you please re-base this PR against current 'master'?

Will do asap

@MarcoRossignoli
Copy link
Member Author

@davidsh rebased!

davidsh
davidsh approved these changes Feb 22, 2020
View reviewed changes
Copy link
Contributor

@davidsh davidsh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR!

@davidsh davidsh requested a review from scalablecory February 22, 2020 21:28
@davidsh
Copy link
Contributor

davidsh commented Feb 22, 2020

@scalablecory Can you confirm that your requested changes/feedback was addressed? If so, please approve PR. Thx.

@scalablecory
Copy link
Contributor

/azp run runtime-libraries outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

scalablecory
scalablecory approved these changes Feb 25, 2020
View reviewed changes
Copy link
Contributor

@scalablecory scalablecory left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Thanks for the PR.

@davidsh davidsh merged commit 23e388b into dotnet:master Feb 26, 2020
@bergmeister bergmeister mentioned this pull request Apr 7, 2020
Merged
14 tasks
@ghost ghost locked as resolved and limited conversation to collaborators Dec 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@tmds tmds tmds left review comments

@karelz karelz karelz left review comments

@scalablecory scalablecory scalablecory approved these changes

@davidsh davidsh davidsh approved these changes

Assignees
No one assigned
Labels
area-System.Net
Projects
None yet
Milestone
5.0.0
Development

Successfully merging this pull request may close these issues.
7 participants
@MarcoRossignoli @MihaZupan @scalablecory @davidsh @tmds @karelz @Dotnet-GitSync-Bot