Support cancellation on SslStream.AuthenticateAs*Async methods

[stephentoub]

cc: @Priya91
Previous attempt in dotnet/corefx#24857 reverted in dotnet/corefx#25203 due to it causing a failing outerloop test SslStream_StreamToStream_WriteAsync_ReadAsync_Pending_Success (outerloop tests weren't run on the original PR).

[Priya91]

@stephentoub @Drawaes Is the cancellationToken PR the cause for the failing test? The test is question is not marked as outerloop, so it was run on the cancellationToken PR. Is there any other information from the investigation as to the cause?

[Priya91]

@Drawaes Also this test failure happens on only on Linux right? The cancellation token change was entirely os agnostic. I'm trying to understand how it was verified that this was the cause, it'll be helpful if you could share more information on the investigation, for me work on this further.

[Drawaes]

I ran the outerloop with the commits... consistent failures. Took the commits out and it passes.

Locally master could bot run a benchmark on asp.net. if I took those commits out asp.net worked fine ( this was Ubuntu 16.04)

[Priya91]

Since the test was not marked outerloop, but the failure happens only in outerloop, maybe someother outerloop test is interfering with this, or the cancellation token change exposed some other bug. More investigation is required, i'll look into this.

Locally master could bot run a benchmark on asp.net. if I took those commits out asp.net worked fine

I don't understand what you mean by this.. If you have a short repro, I can use that to debug locally.

[stephentoub]

I don't understand what you mean by this

He means with the change running ASP.NET benchmarks resulting in hangs.

[Drawaes]

I used https://github.com/benaadams/netperf/tree/aspnet/AspNetCore
which is as basic a webserver as you can make. Then I used net core 2.1 from master. It didn't actually hang, it starts the ssl handshake and then drops the connection before it actually sends data. It does it on every connect so you can just hit it with any browser on 5001 and it gives you an error.

Removing these commits it could do 800k requests per second with zero failures. I did all of this on a clean machine in Azure (16 Cores Ubuntu 16.04). From memory though the perf test that @geoffkizer has failed also that doesn't have asp.net in the loop at all which might be an easier repo

https://github.com/geoffkizer/netperf/tree/master/SslStreamPerf

Just using the loop back had the same issue I am pretty sure,

[Priya91]

One difference I see from the previous commits in this codepath that has varied is in HandleWriteCallback, prior to the writeasync overload work on sslstream, in FinishHandshake, the work done on the thread pool is CompleteRequestWaitCallback, upon refactoring, this is now doing AsyncResumeHandshake in this commit. @Drawaes Is this change intentional?

[Drawaes]

Doesn't seem so I will need to double check tomorrow. If you change it does it fix it?

Looking at that code, the original "write" path won't hit this anyway as it uses a tsc. Also this should only happen if you are trying to write during a handshake which in the repro code would only happen during a handshake re-negotiation

[Drawaes]

Actually that case isn't even needed it's only ever tcs or lazyasyncresult so that case could just be deleted

[Drawaes]

Actually it is set in one place, need to look on a computer

[Priya91]

I've not been able to repro the test failure locally, I'm currently just reading code to understand what;s happening. The cancellation code path causing hang is also weird, as that code is hit only during handshake, and the failing test is hanging after the handshake, in SslStream.EndRead from the callstack on the issue #21669

And the HandleWriteCallback code change is also suspicious, because as it stand now, in some scenario, it is doing a asyncresumehandshake after a finishhandshake, completely opposite of what it did previously which was setting the async operation as complete. So it is re-initiating handshake in a separate thread pool thread.

[Priya91]

@Drawaes Are you able to repro the hang in the test consistently locally? Can you share your repro steps..

[Drawaes]

I need a branch I can checkout with your changes back in it and I will then repo it, did you try the asp.net example?

[Drawaes]

Looking at that change, it was fine. The reason for the single path through the async is because the queueing of the async write changed to a TCS, therefore it should never hit that case if it is an queued write, only a queued handshake.

Is the cancellation code adding APM code back into the new Async/Await write path?

[Priya91]

Is the cancellation code adding APM code back into the new Async/Await write path?

The cancellation change is just adding an extra field on asyncprotocolrequest object that is passed down through the existing handshake methods in sslstate. During re-negotiation at the time of read/write, the handshake codepath will be hit which is having the apm pattern. Although the cancellation token will be none, so the cancellationtoken change will not be hit, as the if will be returning false everywhere. The bug most likely exists in the read/write codepath, but it would be good to get a repro first, so it's easier to debug.