Implement cancellation token for SslStream new AuthenticateAs*Async methods by Priya91 · Pull Request #24857 · dotnet/corefx

Priya91 · 2017-10-25T02:22:41Z

also fixes #24853

stephentoub · 2017-10-25T02:46:31Z

        /// Otherwise, reads as directed or completes "request" with an Exception.
        /// </summary>
-        public static async void ReadPacketAsync(Stream transport, AsyncProtocolRequest request) // "async Task" might result in additional, unnecessary allocation
+        public static async void ReadPacketAsync(Stream transport, AsyncProtocolRequest request, CancellationToken cancellationToken) // "async Task" might result in additional, unnecessary allocation


As long as you're editing this line, you can remove the comment at the end; that's no longer the case.

stephentoub · 2017-10-25T02:47:50Z

-                ForceAuthentication(Context.IsServer, null, asyncRequest);
+                if (asyncRequest != null)
+                {
+                    cancellationToken.ThrowIfCancellationRequested();


Cancellation exceptions shouldn't be thrown out of the invocations synchronously; they should be passed out in a task. That's going to require more refactoring, e.g. to do what @Drawaes has been doing and converting things from being Task-over-APM to instead be APM-over-Task, at which point implementing the cancellation support in that way will be trivial.

stephentoub · 2017-10-25T02:48:53Z

            _Framing = Framing.Unknown;

+            // Throw if cancellation requested.
+            cancellationToken.ThrowIfCancellationRequested();


Pretty much any of these places where you've got a ThrowIfCancellationRequested and it's not an async method is likely not going to do the right thing, as the exception will be thrown out synchronously rather than propagate out through the returned Task.

Also, do we need all of these polling checks for cancellation? Seems like there should be a single check up front and then it should just be passed to all of the stream operations.

That assumes the underlying stream fully implements cancellation.

That assumes the underlying stream fully implements cancellation.

No, it doesn't. It assumes the underlying stream at least does a single check for cancellation, which most every implementation does... even the base stream's implementation layered on Begin/End or the sync methods does so. We should not litter polling checks everywhere.

stephentoub · 2017-10-25T02:50:35Z

                else
                {
+                    // Throw before starting async write
+                    cancellationToken.ThrowIfCancellationRequested();


Why this rather than passing the cancellation token into the WriteAsync call?

stephentoub · 2017-10-25T02:54:33Z

+                serverOptions.ServerCertificate = certificate;
+                serverOptions.RemoteCertificateValidationCallback = AllowAnyServerCertificate;
+
+                CancellationTokenSource cts = new CancellationTokenSource();


What's the purpose of invoking Cancel in a Task? This is introducing a race condition such that we won't always be testing the same thing. If the goal is to have a canceled token before we call the operations, you can either just call cts.Cancel() synchronously or just use new CancellationToken(true). If the goal is to have cancellation occur once the operation is in progress, it'd be better to make the cts.Cancel() call synchronously below after you've got the clientTask and serverTask; that way, you know the cancellation request is coming in after all of the synchronous work done by those methods while they're actually waiting on IO.

stephentoub · 2017-10-25T02:54:46Z

+                serverOptions.ServerCertificate = certificate;
+                serverOptions.RemoteCertificateValidationCallback = AllowAnyServerCertificate;
+
+                CancellationTokenSource cts = new CancellationTokenSource();


stephentoub · 2017-10-25T02:55:37Z

+        public void SslStream_StreamToStream_ClientCancellation_Throws()
+        {
+            VirtualNetwork network = new VirtualNetwork();
+            using (var clientStream = new VirtualNetworkStream(network, false))


Nit: please name the bool args to VirtualNetworkStream; it's not clear at the call site what the false and true mean.

stephentoub · 2017-10-25T02:56:04Z

                }

-                sslState.CheckCompletionBeforeNextReceive((ProtocolToken)asyncState, asyncRequest);
+                // Not allowing cancellation in the callback.


stephentoub · 2017-10-25T02:57:09Z

        {
+            if (asyncRequest != null)
+            {
+                cancellationToken.ThrowIfCancellationRequested();


What guarantees if any do we make if authentication is canceled in the middle of it? Presumably no guarantees and at that point you can't use the SslStream for anything else?

Yes, the exception thrown for cancellation will fail the handshake, and make the SslStream invalid.

stephentoub · 2017-11-02T03:55:23Z

+            if (asyncRequest != null && asyncRequest.CancellationToken.IsCancellationRequested)
+            {
+                // Cancel async operation, before I/O starts.
+                asyncRequest.CompleteUserWithError(new TaskCanceledException());


Nit: instead of new TaskCanceledException(), it'd be better to do new OperationCanceledException(asyncRequest.CancellationToken), so that the token that caused the cancellation is included in the exception.

(Same applies to the other cases of this elsewhere.)

So I had that initially, and when I commented out all of the OperationCanceledException instance, and made cancellation throw from the InnerStream.WriteAsync/ReadAsync methods, it threw TaskCanceledException. Hence made these the same, to have uniform cancellation exception.

stephentoub · 2017-11-02T03:56:37Z

        {
+            if (asyncRequest != null && asyncRequest.CancellationToken.IsCancellationRequested)
+            {
+                // Return async operation if cancellation requested.


Nit: "Return" => "Cancel"?

stephentoub

Thanks, @Priya91.

stephentoub · 2017-11-03T14:04:38Z

Ubuntu failure:

Unhandled Exception of Type System.AggregateException
Message :
System.AggregateException : One or more errors occurred. (Assert.Throws() Failure
Expected: typeof(System.TimeoutException)
Actual:   typeof(Xunit.Sdk.EqualException): Assert.Equal() Failure
Expected: RemoteCertificateChainErrors
Actual:   RemoteCertificateNotAvailable)
---- Assert.Throws() Failure
Expected: typeof(System.TimeoutException)
Actual:   typeof(Xunit.Sdk.EqualException): Assert.Equal() Failure
Expected: RemoteCertificateChainErrors
Actual:   RemoteCertificateNotAvailable
Stack Trace :
   at System.Threading.Tasks.Task.WaitAllCore(Task[] tasks, Int32 millisecondsTimeout, CancellationToken cancellationToken) in /root/coreclr/src/mscorlib/src/System/Threading/Tasks/Task.cs:line 4651
   at System.Net.Security.Tests.SslStreamAlpnTests.SslStream_StreamToStream_ClientCancellation_Throws() in /mnt/j/workspace/dotnet_corefx/master/linux-TGroup_netcoreapp+CGroup_Release+AGroup_x64+TestOuter_false_prtest/src/System.Net.Security/tests/FunctionalTests/SslStreamAlpnTests.cs:line 86
----- Inner Stack Trace -----
   at System.Net.Security.Tests.SslStreamAlpnTests.AllowAnyServerCertificate(Object sender, X509Certificate certificate, X509Chain chain, SslPolicyErrors sslPolicyErrors) in /mnt/j/workspace/dotnet_corefx/master/linux-TGroup_netcoreapp+CGroup_Release+AGroup_x64+TestOuter_false_prtest/src/System.Net.Security/tests/FunctionalTests/SslStreamAlpnTests.cs:line 53
   at System.Net.Security.SecureChannel.VerifyRemoteCertificate(RemoteCertValidationCallback remoteCertValidationCallback, ProtocolToken& alertToken) in /mnt/j/workspace/dotnet_corefx/master/linux-TGroup_netcoreapp+CGroup_Release+AGroup_x64+TestOuter_false_prtest/src/System.Net.Security/src/System/Net/Security/SecureChannel.cs:line 1026
   at System.Net.Security.SslState.CompleteHandshake(ProtocolToken& alertToken) in /mnt/j/workspace/dotnet_corefx/master/linux-TGroup_netcoreapp+CGroup_Release+AGroup_x64+TestOuter_false_prtest/src/System.Net.Security/src/System/Net/Security/SslState.cs:line 1054
   at System.Net.Security.SslState.CheckCompletionBeforeNextReceive(ProtocolToken message, AsyncProtocolRequest asyncRequest) in /mnt/j/workspace/dotnet_corefx/master/linux-TGroup_netcoreapp+CGroup_Release+AGroup_x64+TestOuter_false_prtest/src/System.Net.Security/src/System/Net/Security/SslState.cs:line 830
   at System.Net.Security.SslState.WriteCallback(IAsyncResult transportResult) in /mnt/j/workspace/dotnet_corefx/master/linux-TGroup_netcoreapp+CGroup_Release+AGroup_x64+TestOuter_false_prtest/src/System.Net.Security/src/System/Net/Security/SslState.cs:line 1113
--- End of stack trace from previous location where exception was thrown ---
   at System.Net.Security.SslState.EndProcessAuthentication(IAsyncResult result) in /mnt/j/workspace/dotnet_corefx/master/linux-TGroup_netcoreapp+CGroup_Release+AGroup_x64+TestOuter_false_prtest/src/System.Net.Security/src/System/Net/Security/SslState.cs:line 718
   at System.Threading.Tasks.TaskFactory`1.FromAsyncCoreLogic(IAsyncResult iar, Func`2 endFunction, Action`1 endAction, Task`1 promise, Boolean requiresSynchronization) in /root/coreclr/src/mscorlib/src/System/Threading/Tasks/FutureFactory.cs:line 533
--- End of stack trace from previous location where exception was thrown ---

Priya91 · 2017-11-03T18:18:07Z

I had set the RemoteCertificateValidationCallback on server options as well, and the client was not sending any clientcertificates, hence the error about RemoteCertificateNotAvailable. But this verification happens only in CompleteHandshake phase, which means the server didn't timeout before that, so the cancellation didn't happen during the handshake. This could also result in flaky test, hence movign the cancellation trigger before starting the client and server tasks.

Priya91 · 2017-11-03T21:04:47Z

The windows test failure is unrelated to this change, in System.Diagnostics.EventLog.Tests

System.ArgumentException : Index -1 is out of bounds.
Stack Trace :
   at System.Diagnostics.EventLogInternal.GetEntryAt(Int32 index) in D:\j\workspace\windows-TGrou---f8ac6754\src\System.Diagnostics.EventLog\src\System\Diagnostics\EventLogInternal.cs:line 880
   at System.Diagnostics.EventLogEntryCollection.get_Item(Int32 index) in D:\j\workspace\windows-TGrou---f8ac6754\src\System.Diagnostics.EventLog\src\System\Diagnostics\EventLogEntryCollection.cs:line 30
   at System.Diagnostics.Tests.EventLogEntryCollectionTests.<>c__DisplayClass4_1.<CheckingEntryInEquality>b__3() in D:\j\workspace\windows-TGrou---f8ac6754\src\System.Diagnostics.EventLog\tests\EventLogEntryCollectionTests.cs:line 113
   at System.Diagnostics.Tests.Helpers.RetryOnWin7[T](Func`1 func) in D:\j\workspace\windows-TGrou---f8ac6754\src\System.Diagnostics.EventLog\tests\Helpers.cs:line 36
   at System.Diagnostics.Tests.EventLogEntryCollectionTests.CheckingEntryInEquality() in D:\j\workspace\windows-TGrou---f8ac6754\src\System.Diagnostics.EventLog\tests\EventLogEntryCollectionTests.cs:line 113

stephentoub · 2017-11-03T21:38:00Z

@Priya91, please make sure you run outerloop tests before merging any more changes like this. Thanks.

Implement cancellation token for SslStream new AuthenticateAs*Async methods Commit migrated from dotnet/corefx@7cbb09b

dnfclas added the cla-already-signed label Oct 25, 2017

stephentoub reviewed Oct 25, 2017

View reviewed changes

davidsh added the area-System.Net.Security label Oct 28, 2017

karelz assigned Priya91 Oct 28, 2017

karelz added this to the 2.1.0 milestone Oct 28, 2017

Priya91 force-pushed the cancellation branch from 58bc8b4 to d786c0d Compare November 1, 2017 02:37

Lakshmi Priya Sekar added 2 commits November 1, 2017 11:41

Implement cancellation policy for sslstream.

f2c8a55

Fix exceptions on failing alpn protocol mismatch test.

d5e72b1

Priya91 force-pushed the cancellation branch from d786c0d to d5e72b1 Compare November 1, 2017 18:41

stephentoub reviewed Nov 2, 2017

View reviewed changes

stephentoub approved these changes Nov 2, 2017

View reviewed changes

Change TaskCanceledException to OperationCancelledException.

cbc0b59

stephentoub approved these changes Nov 3, 2017

View reviewed changes

Fix flaky test.

b2e9cd1

Priya91 merged commit 7cbb09b into dotnet:master Nov 3, 2017

Priya91 deleted the cancellation branch November 3, 2017 21:05

Drawaes mentioned this pull request Nov 11, 2017

[WIP] SslStream Throttle #25187

Closed

stephentoub mentioned this pull request Nov 12, 2017

Revert "Implement cancellation token for SslStream new AuthenticateAs*Async methods" #25203

Merged

stephentoub mentioned this pull request Jan 31, 2020

Support cancellation on SslStream.AuthenticateAs*Async methods dotnet/runtime#24116

Closed

picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022

Merge pull request dotnet/corefx#24857 from Priya91/cancellation

03e849f

Implement cancellation token for SslStream new AuthenticateAs*Async methods Commit migrated from dotnet/corefx@7cbb09b

Uh oh!

Conversation

Priya91 commented Oct 25, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephentoub Oct 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephentoub Nov 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephentoub left a comment

Choose a reason for hiding this comment

Uh oh!

stephentoub commented Nov 3, 2017

Uh oh!

Priya91 commented Nov 3, 2017

Uh oh!

Priya91 commented Nov 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stephentoub commented Nov 3, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

stephentoub Oct 25, 2017 •

edited

Loading

stephentoub Nov 2, 2017 •

edited

Loading

Priya91 commented Nov 3, 2017 •

edited

Loading