Skip to content

SmtpClient SendMailAsync with CancellationToken API implementation - #287

Merged
MihaZupan merged 9 commits into
dotnet:masterfrom
MihaZupan:smtp-client-cancellation
Nov 27, 2019
Merged

SmtpClient SendMailAsync with CancellationToken API implementation#287
MihaZupan merged 9 commits into
dotnet:masterfrom
MihaZupan:smtp-client-cancellation

Conversation

@MihaZupan

@MihaZupan MihaZupan commented Nov 26, 2019

Copy link
Copy Markdown
Member

3f9f0e7 is the implementation of the API introduced in dotnet/corefx#24475

class System.Net.Mail.SmtpClient
{
    public Task SendMailAsync(string from, string recipients, string subject, string body)
+   public Task SendMailAsync(string from, string recipients, string subject, string body, CancellationToken cancellationToken)
    public Task SendMailAsync(MailMessage message)
+   public Task SendMailAsync(MailMessage message, CancellationToken cancellationToken)
}

The rest of the PR are SmtpClient tests using a mock SmtpServer.

CC @dotnet/ncl

@MihaZupan

MihaZupan commented Nov 26, 2019 •
edited
Loading

Copy link
Copy Markdown
Member Author

e513058#diff-e2b2d690217e90426b121345ba65abdcR377 is failing in CI as the test now actually checks that NTLM is used.

Looks like it is essentially this: https://github.com/dotnet/corefx/issues/28961.

Stack trace for reference:

System.Net.Mail.SmtpException: Failure sending mail.
 ---> System.ComponentModel.Win32Exception (0x80090020): GSSAPI operation failed with error - An unsupported mechanism was requested. NTLM authentication requires the GSSAPI plugin 'gss-ntlmssp'.
   at System.Net.Security.NegotiateStreamPal.AcquireCredentialsHandle(String package, Boolean isServer, NetworkCredential credential)
   at System.Net.NTAuthentication.Initialize(Boolean isServer, String package, NetworkCredential credential, String spn, ContextFlagsPal requestedContextFlags, ChannelBinding channelBinding)
   at System.Net.Mail.SmtpNtlmAuthenticationModule.Authenticate(String challenge, NetworkCredential credential, Object sessionCookie, String spn, ChannelBinding channelBindingToken)
   at System.Net.Mail.SmtpConnection.SetContextAndTryAuthenticate(ISmtpAuthenticationModule module, NetworkCredential credential, ContextAwareResult context)
   at System.Net.Mail.SmtpConnection.ConnectAndHandshakeAsyncResult.Authenticate()
   at System.Net.Mail.SmtpConnection.ConnectAndHandshakeAsyncResult.SendEHelloCallback(IAsyncResult result)
...

davidsh
davidsh reviewed Nov 26, 2019
View reviewed changes
Comment thread src/libraries/System.Net.Mail/tests/Functional/SmtpClientTest.cs Outdated
eiriktsarpalis
eiriktsarpalis reviewed Nov 26, 2019
View reviewed changes
Comment thread src/libraries/System.Net.Mail/src/System/Net/Mail/SmtpClient.cs Outdated
eiriktsarpalis
eiriktsarpalis reviewed Nov 26, 2019
View reviewed changes
Comment thread src/libraries/System.Net.Mail/src/System/Net/Mail/SmtpClient.cs Outdated
eiriktsarpalis
eiriktsarpalis reviewed Nov 26, 2019
View reviewed changes
Comment thread src/libraries/System.Net.Mail/src/System/Net/Mail/SmtpClient.cs Outdated
eiriktsarpalis
eiriktsarpalis reviewed Nov 26, 2019
View reviewed changes
Comment thread src/libraries/System.Net.Mail/tests/Functional/MockSmtpServer.cs Outdated
davidsh
davidsh reviewed Nov 26, 2019
View reviewed changes
Comment thread src/libraries/System.Net.Mail/tests/Functional/MockSmtpServer.cs Outdated
davidsh
davidsh reviewed Nov 26, 2019
View reviewed changes
Comment thread src/libraries/System.Net.Mail/tests/Functional/SmtpClientTest.cs Outdated
davidsh
davidsh reviewed Nov 26, 2019
View reviewed changes
Comment thread src/libraries/System.Net.Mail/tests/Functional/SmtpClientTest.cs Outdated
davidsh
davidsh reviewed Nov 26, 2019
View reviewed changes
Comment thread src/libraries/System.Net.Mail/tests/Functional/SmtpClientTest.cs Outdated
@davidsh davidsh added this to the 5.0 milestone Nov 26, 2019
@davidsh
davidsh requested a review from stephentoub November 26, 2019 22:48
@davidsh

davidsh commented Nov 26, 2019

Copy link
Copy Markdown
Contributor

@stephentoub PTAL the use of CancellationTokenRegistration.

@MihaZupan
MihaZupan force-pushed the smtp-client-cancellation branch from b2c94d1 to 6076372 Compare November 27, 2019 00:32
stephentoub
stephentoub reviewed Nov 27, 2019
View reviewed changes
Comment thread src/libraries/System.Net.Mail/src/System/Net/Mail/SmtpClient.cs Outdated
stephentoub
stephentoub reviewed Nov 27, 2019
View reviewed changes
Comment thread src/libraries/System.Net.Mail/tests/Functional/FakeSmtpServer.cs Outdated
@MihaZupan
Address PR feedback
248cbfd
@MihaZupan
Address PR feedback
499a07e 
Use Interlocked.Exchange instead of locks
@MihaZupan
MihaZupan force-pushed the smtp-client-cancellation branch from 6076372 to 499a07e Compare November 27, 2019 11:02
@ManickaP

ManickaP commented Nov 27, 2019

Copy link
Copy Markdown
Member

/azp run runtime-libraries outerloop

@azure-pipelines

azure-pipelines Bot commented Nov 27, 2019

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@stephentoub

stephentoub commented Nov 27, 2019

Copy link
Copy Markdown
Member

The src changes LGTM. Thanks.

davidsh
davidsh approved these changes Nov 27, 2019
View reviewed changes

@davidsh davidsh left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM. You'll want to check the Outerloop CI results to verify the failures aren't related to the PR.

@MihaZupan

MihaZupan commented Nov 27, 2019

Copy link
Copy Markdown
Member Author

Outerloop failures are unrelated, Mail itself doesn't have any OL tests.

@davidsh

davidsh commented Nov 27, 2019

Copy link
Copy Markdown
Contributor

Outerloop failures are unrelated, Mail itself doesn't have any OL tests.

In that case, you are free to 'Squash and merge' this unless you want additional feedback from others.

@MihaZupan

MihaZupan commented Nov 27, 2019

Copy link
Copy Markdown
Member Author

Thanks for the reviews!

@MihaZupan
MihaZupan merged commit 40b11db into dotnet:master Nov 27, 2019
MichalStrehovsky pushed a commit to MichalStrehovsky/runtime that referenced this pull request Nov 10, 2020
@ChiragRupani
Changed References from NonGCStaticsNode GetClassConstructorContextSt…
4cc166d 
…orageSize to GetClassConstructorContextSize (dotnet#287)
@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

@eiriktsarpalis eiriktsarpalis eiriktsarpalis left review comments
@stephentoub stephentoub Awaiting requested review from stephentoub
+1 more reviewer
@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.

6 participants

@MihaZupan @davidsh @ManickaP @stephentoub @eiriktsarpalis @Dotnet-GitSync-Bot