Skip to content

Send QUIT on SmtpClient.Dispose()#683

Merged
davidsh merged 6 commits intodotnet:masterfrom
MarcoRossignoli:smtpquit
Feb 26, 2020
Merged

Send QUIT on SmtpClient.Dispose()#683
davidsh merged 6 commits intodotnet:masterfrom
MarcoRossignoli:smtpquit

Conversation

@MarcoRossignoli
Copy link
Copy Markdown
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
Copy Markdown
Member

MihaZupan commented Dec 9, 2019

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
Copy Markdown
Member

MihaZupan commented Dec 9, 2019

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
Copy Markdown
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
Copy Markdown
Contributor

scalablecory commented Dec 9, 2019

/azp run

@azure-pipelines
Copy link
Copy Markdown

azure-pipelines bot commented Dec 9, 2019

Azure Pipelines successfully started running 10 pipeline(s).

@davidsh
Copy link
Copy Markdown
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
Copy Markdown
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
davidsh
davidsh reviewed Dec 17, 2019
View reviewed changes
@MarcoRossignoli MarcoRossignoli mentioned this pull request Dec 17, 2019
Closed
tmds
tmds reviewed Dec 17, 2019
View reviewed changes
tmds
tmds reviewed Dec 17, 2019
View reviewed changes
@MarcoRossignoli
Copy link
Copy Markdown
Member Author

MarcoRossignoli commented Dec 18, 2019

/azp list

@azure-pipelines
Copy link
Copy Markdown

azure-pipelines bot commented Dec 18, 2019

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

@MarcoRossignoli
Copy link
Copy Markdown
Member Author

MarcoRossignoli commented Jan 16, 2020

/azp list

@azure-pipelines
Copy link
Copy Markdown

azure-pipelines bot commented Jan 16, 2020

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
Copy Markdown
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.

tmds
tmds reviewed Jan 17, 2020
View reviewed changes
@MarcoRossignoli
Copy link
Copy Markdown
Member Author

MarcoRossignoli commented Jan 17, 2020

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

Would be great 😍

@scalablecory
Copy link
Copy Markdown
Contributor

scalablecory commented Jan 27, 2020

/azp run runtime-libraries outerloop

@azure-pipelines
Copy link
Copy Markdown

azure-pipelines bot commented Jan 27, 2020

Azure Pipelines successfully started running 1 pipeline(s).

tmds
tmds reviewed Jan 27, 2020
View reviewed changes
MarcoRossignoli
MarcoRossignoli commented Feb 7, 2020
View reviewed changes
src/libraries/System.Net.Mail/src/System/Net/Mail/SmtpClient.cs Outdated
Copy link
Copy Markdown
Member Author

@MarcoRossignoli MarcoRossignoli Feb 7, 2020

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 Outdated
Copy link
Copy Markdown
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
Copy Markdown
Member Author

MarcoRossignoli commented Feb 15, 2020

/azp list

@azure-pipelines
Copy link
Copy Markdown

azure-pipelines bot commented Feb 15, 2020

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

@davidsh
Copy link
Copy Markdown
Contributor

davidsh commented Feb 15, 2020

/azp list

@azure-pipelines
Copy link
Copy Markdown

azure-pipelines bot commented Feb 15, 2020

CI/CD Pipelines for this repository:

@davidsh
Copy link
Copy Markdown
Contributor

davidsh commented Feb 15, 2020

/azp run runtime-libraries outerloop

@azure-pipelines
Copy link
Copy Markdown

azure-pipelines bot commented Feb 15, 2020

Azure Pipelines successfully started running 1 pipeline(s).

@davidsh
Copy link
Copy Markdown
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
Copy Markdown
Member Author

MarcoRossignoli commented Feb 21, 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'?

Will do asap

@MarcoRossignoli
Copy link
Copy Markdown
Member Author

MarcoRossignoli commented Feb 22, 2020

@davidsh rebased!

davidsh
davidsh approved these changes Feb 22, 2020
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor

scalablecory commented Feb 25, 2020

/azp run runtime-libraries outerloop

@azure-pipelines
Copy link
Copy Markdown

azure-pipelines bot commented Feb 25, 2020

Azure Pipelines successfully started running 1 pipeline(s).

scalablecory
scalablecory approved these changes Feb 25, 2020
View reviewed changes
Copy link
Copy Markdown
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

@karelz karelz karelz left review comments

+3 more reviewers

@tmds tmds tmds left review comments

@scalablecory scalablecory scalablecory approved these changes

@davidsh davidsh davidsh approved these changes

Reviewers whose approvals may not affect merge requirements

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