Porting async implementation and adding TPL overrides for SslStream Read/WriteAsync #5541

CIPop · 2016-01-20T02:07:19Z

Porting APM functionality for SslStream's InnerStream
Adding TPL wrappers
Adding the test proposed by @xanather

Fixes #5077

@stephentoub @davidsh @bartonjs @vijaykota PTAL

After review, I'll follow the internal process for RC2.

CIPop · 2016-01-20T18:08:15Z

@benaadams If you have some time, it would be interesting to check if this fixes your issue while we wait for the PR review.

benaadams · 2016-01-20T19:09:10Z

@CIPop can I use as is (e.g. I don't need the other Stream changes in this area) - will give it a go.

CIPop · 2016-01-20T19:47:53Z

@CIPop can I use as is (e.g. I don't need the other Stream changes in this area) - will give it a go.

@benaadams If your code is using a true async stream (e.g. NetworkStream), I don't think you need the Stream enhancements for this to unblock you.

benaadams · 2016-01-20T19:53:44Z

@CIPop its pure async; with the sync being simulated - which is why it interacted quite badly with the previous Stream's async overlay over the sync methods - ended up ticking all the "don't do" boxes of async rather than just the one 😁 e.g. it was sync over async and async over sync at the same time.

davidsh · 2016-01-20T20:00:10Z

src/System.Net.Security/tests/FunctionalTests/SslStreamNetworkStreamTest.cs

+        public async void SslStream_SendReceiveOverNetworkStream_Ok()
+        {
+            X509Certificate2 serverCertificate = TestConfiguration.GetServerCertificate();
+            TcpListener listener = new TcpListener(IPAddress.Any, 4567);


We should change the port from a fixed '4567' to a dynamic port, i.e. '0'. Otherwise, if a single CI machine is running multiple instances of PR validation, this port might be busy running another test.

Good point. I've missed that when I was adapting the test for CI.

benaadams · 2016-01-20T21:26:45Z

@CIPop I'm having trouble building it - though I think its a general corefx thing, so will start from scratch.

As an aside the CI tests can't compile e.g.

Error   CS1061  '_SslStream' does not contain a definition for 'BeginRead' and no extension 
method 'BeginRead' accepting a first argument of type '_SslStream' could be found (are you missing a using directive or an assembly reference?) 
System.Net.Security.Unit.Tests  I:\Work\GitHub\corefx\src\System.Net.Security\src\System\Net\SecureProtocols\SslStream.cs   446

CIPop · 2016-01-20T21:37:56Z

CI tests can't compile

Thanks @benaadams. Addressed that and will update the PR shortly. (The UT Fakes needed updated.)

benaadams · 2016-01-21T02:26:16Z

@CIPop I can't build, I think I might be between tools; though it starts with this

I:\corefx>build.cmd
Building Native Libraries...
'&' is not recognized as an internal or external command,
operable program or batch file.
'I:\corefx\src\Native\Windows\probe-win.ps1 is not digitally signed. You cannot run this script on the current system. ' is not recognized as an internal or external command,
operable program or batch file.
'For' is not recognized as an internal or external command,
operable program or batch file.
'http:' is not recognized as an internal or external command,
operable program or batch file.
The system cannot find the path specified.
'+   ~~~~~~~~~~~~~~~' is not recognized as an internal or external command,
operable program or batch file.
The filename, directory name, or volume label syntax is incorrect.
The filename, directory name, or volume label syntax is incorrect.
'""' is not recognized as an internal or external command,
operable program or batch file.
Native component build failed see nativebuild.log for more details.
  Restoring all packages...

It does build some stuff though. If you could put the dll and pdb somewhere I could give it a go.

stephentoub · 2016-01-21T13:24:42Z

src/System.Net.Security/src/System/Net/SecureProtocols/SslStreamInternal.cs

+        private static AsyncProtocolCallback _resumeAsyncWriteCallback = new AsyncProtocolCallback(ResumeAsyncWriteCallback);
+        private static AsyncProtocolCallback _resumeAsyncReadCallback = new AsyncProtocolCallback(ResumeAsyncReadCallback);
+        private static AsyncProtocolCallback _readHeaderCallback = new AsyncProtocolCallback(ReadHeaderCallback);
+        private static AsyncProtocolCallback _readFrameCallback = new AsyncProtocolCallback(ReadFrameCallback);


Nit: These could all be readonly. They should also have an s_ prefix.

Thanks! Fixed.

stephentoub · 2016-01-21T13:38:04Z

LGTM

stephentoub · 2016-01-21T13:38:14Z

The two Windows_NT failures are:

14:20:38      System.Net.Security.Tests.SslStreamStreamToStreamTest.SslStream_StreamToStream_Successive_ClientWrite_Sync_Success [FAIL]
14:20:38         Handshake completed.
14:20:38         Expected: True
14:20:38         Actual:   False
14:20:38         Stack Trace:
14:20:38            d:\j\workspace\dotnet_corefx\windows_nt_debug_prtest\src\System.Net.Security\tests\FunctionalTests\SslStreamStreamToStreamTest.cs(76,0): at System.Net.Security.Tests.SslStreamStreamToStreamTest.SslStream_StreamToStream_Successive_ClientWrite_Sync_Success()
14:20:38      System.Net.Security.Tests.ClientAsyncAuthenticateTest.ClientAsyncAuthenticate_ServerNoEncryption_NoConnect [FAIL]
14:20:38         Assert.Throws() Failure
14:20:38         Expected: typeof(System.IO.IOException)
14:20:38         Actual:   typeof(Xunit.Sdk.TrueException): Timed Out
14:20:38         Expected: True
14:20:38         Actual:   False
14:20:38         Stack Trace:
14:20:38            d:\j\workspace\dotnet_corefx\windows_nt_debug_prtest\src\System.Net.Security\tests\FunctionalTests\ClientAsyncAuthenticateTest.cs(172,0): at System.Net.Security.Tests.ClientAsyncAuthenticateTest.<ClientAsyncSslHelper>d__18.MoveNext()
14:20:38            --- End of stack trace from previous location where exception was thrown ---
14:20:38               at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
14:20:38               at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)

CIPop · 2016-01-22T00:54:30Z

@stephentoub

The two Windows_NT failures are

Since they are intermittent, I believe these are #4467. I found a few issues with both the test patterns and the Mock infra.

CIPop · 2016-01-22T00:56:43Z

I can't build, I think I might be between tools; though it starts with this

@benaadams I don't see these errors but I think you're using PoshGit. Try to open a cmd command prompt instead.

…ppers and a simple test proposed by the community.

davidsh · 2016-01-22T18:33:46Z

LGTM

Porting async implementation and adding TPL overrides for SslStream Read/WriteAsync

CIPop · 2016-01-22T18:35:31Z

Thanks for the review!

benaadams · 2016-01-26T17:41:38Z

@CIPop still having a lock up post this fix; debugging into it (need to determine cause using v4.0.0-rc2-23722)

benaadams · 2016-01-26T20:14:01Z

@CIPop still seem to be dropping into a System.IO.Stream Sync read in BeginReadInternal when running on framework net451 framework

benaadams · 2016-01-26T20:20:47Z

Call stack

CIPop · 2016-01-26T20:35:25Z

@benaadams This fix is only for corefx. net451 would use .Net Desktop implementations. Can you repro this on CoreFX?

benaadams · 2016-01-26T21:48:38Z

Trying

benaadams · 2016-01-27T01:14:17Z

Yes; on 32-bit DNXCore 5.0 though trying to workout how to get some debug info to find out what's going on; might be a different cause - will keep you posted.

The current clr/coreclr I'm using is:

#if DOTNET5_4
     return WriteAsyncAwaited(buffer, cancellationToken);
#else
     Write(buffer);
     return TaskUtilities.CompletedTask;
#endif

CIPop · 2016-01-27T01:22:14Z

@benaadams For visibility and tracking purposes it would be best to open a different issue and add the details there. It appears that the requirements are different for your issue to repro compared to #5077.

If creating a small repro project isn't practical (and for your current debugging), try collecting ETW logs: https://github.com/dotnet/corefx/blob/master/Documentation/debugging/windows-instructions.md

Porting async implementation and adding TPL overrides for SslStream Read/WriteAsync Commit migrated from dotnet/corefx@fac18d8

CIPop added the area-System.Net label Jan 20, 2016

CIPop self-assigned this Jan 20, 2016

CIPop added this to the 1.0.0-rc2 milestone Jan 20, 2016

dnfclas added the cla-already-signed label Jan 20, 2016

CIPop added the 2 - In Progress label Jan 20, 2016

benaadams mentioned this pull request Jan 20, 2016

StreamSocketOutput WriteAsync as async (SSL) aspnet/KestrelHttpServer#588

Merged

5 tasks

davidsh reviewed Jan 20, 2016
View reviewed changes

stephentoub reviewed Jan 21, 2016
View reviewed changes

stephentoub mentioned this pull request Jan 21, 2016

Refactoring the Windows specific code in NegotiateStream #5581

Closed

CIPop force-pushed the snsrw branch from 6a455e9 to c2b6f7b Compare January 21, 2016 20:55

Porting APM functionality for SslStream's InnerStream. Adding TPL wra…

aa2e8a7

…ppers and a simple test proposed by the community.

CIPop force-pushed the snsrw branch from c2b6f7b to aa2e8a7 Compare January 22, 2016 01:02

CIPop added a commit that referenced this pull request Jan 22, 2016

Merge pull request #5541 from CIPop/snsrw

fac18d8

Porting async implementation and adding TPL overrides for SslStream Read/WriteAsync

CIPop merged commit fac18d8 into dotnet:master Jan 22, 2016

CIPop deleted the snsrw branch January 22, 2016 18:35

CIPop removed the 2 - In Progress label Jan 22, 2016

CIPop mentioned this pull request Jan 26, 2016

RC2: Porting SslStream <Read|Write>Async overrides and regression fix #5691

Merged

stephentoub added the netfx-port-consider label Apr 13, 2016

AlexGhiondea added netfx-port-consider and removed netfx-port-consider labels Apr 13, 2016

benaadams mentioned this pull request Apr 18, 2016

StreamSocketOutput.WriteAsync is Sync (SSL) aspnet/KestrelHttpServer#569

Closed

5 tasks

This was referenced Jan 31, 2020

Tracking items for System.Net.Security dotnet/runtime#15141

Closed

System.Net: Replace existing "catch Exception / throw later" patterns with ExceptionDispatchInfo dotnet/runtime#16191

Closed

vijaykota mentioned this pull request Jan 31, 2020

Additional PR feedback for NegotiateStream porting dotnet/runtime#16195

Closed

16 tasks

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

Merge pull request dotnet/corefx#5541 from CIPop/snsrw

2c567ee

Porting async implementation and adding TPL overrides for SslStream Read/WriteAsync Commit migrated from dotnet/corefx@fac18d8

Porting async implementation and adding TPL overrides for SslStream Read/WriteAsync #5541

Porting async implementation and adding TPL overrides for SslStream Read/WriteAsync #5541

Uh oh!

Conversation

CIPop commented Jan 20, 2016

Uh oh!

CIPop commented Jan 20, 2016

Uh oh!

benaadams commented Jan 20, 2016

Uh oh!

CIPop commented Jan 20, 2016

Uh oh!

benaadams commented Jan 20, 2016

Uh oh!

davidsh Jan 20, 2016

Choose a reason for hiding this comment

Uh oh!

CIPop Jan 20, 2016

Choose a reason for hiding this comment

Uh oh!

benaadams commented Jan 20, 2016

Uh oh!

CIPop commented Jan 20, 2016

Uh oh!

benaadams commented Jan 21, 2016

Uh oh!

stephentoub Jan 21, 2016

Choose a reason for hiding this comment

Uh oh!

CIPop Jan 22, 2016

Choose a reason for hiding this comment

Uh oh!

stephentoub commented Jan 21, 2016

Uh oh!

stephentoub commented Jan 21, 2016

Uh oh!

CIPop commented Jan 22, 2016

Uh oh!

CIPop commented Jan 22, 2016

Uh oh!

davidsh commented Jan 22, 2016

Uh oh!

CIPop commented Jan 22, 2016

Uh oh!

benaadams commented Jan 26, 2016

Uh oh!

benaadams commented Jan 26, 2016

Uh oh!

benaadams commented Jan 26, 2016

Uh oh!

CIPop commented Jan 26, 2016

Uh oh!

benaadams commented Jan 26, 2016

Uh oh!

benaadams commented Jan 27, 2016

Uh oh!

CIPop commented Jan 27, 2016

Uh oh!

Uh oh!