System.Net.Security tests are broken when updating to the new coreclr

[joperezr]

When trying to update to the newest coreclr with PR dotnet/corefx#12836 some System.Net.Security tests started hanging. They all call the same API which launches a task that is awaited and apparently it never resolves. Tests that are in this state are:

SslStream_AuthenticateAsClientAsync_Supported_Success
SslStream_AuthenticateAsClient_Supported_Success
SslStream_AuthenticateAsClientAsync_AllSupported_Success
SslStream_AuthenticateAsClientAsync_None_Success
SslStream_AuthenticateAsClientAsync_Default_Success

I will disable those tests for now to unblock the coreclr update which some folks depend on, but this issue is to track why did those started failing.

cc: @stephentoub @CIPop (people who have edited those tests)

[karelz]

How many tests are you disabling?

[CIPop]

@joperezr Also, do you have more information on what API is hanging (call stack)?

[joperezr]

They are all calling this method:

            protected override void AuthenticateAsClient(
                SslStream stream, bool waitForCompletion,
                string targetHost, X509CertificateCollection clientCertificates, SslProtocols enabledSslProtocols, bool checkCertificateRevocation)
            {
                Task t = stream.AuthenticateAsClientAsync(targetHost, clientCertificates, enabledSslProtocols, checkCertificateRevocation);
                if (waitForCompletion)
                {
                    t.GetAwaiter().GetResult();
                }
            }

And basically the line with t.GetAwaiter().GetResult() is never returning.

[joperezr]

I don't really have a callstack as I didn't really spent much time debugging this, but I thing that debugging the call of stream.AuthenticateAsClientAsync seems to be the key

[CIPop]

@joperezr If Security is the only contract that's failing after this CLR update, please disable the test and assign the issue to me for investigation.
If there are other Task related failures within the same update, it's probably best if it's analyzed by the CLR team before the merge.

I'm also interested to know the answer to @karelz question above:

How many tests are you disabling?

[stephentoub]

How many tests are you disabling?

I believe @joperezr listed out in his original comment the five tests that were failing and that he disabled.

[CIPop]

@stephentoub @joperezr Are those tests the only tests that needed to be disabled during this merge? The original comment is only talking about S.N.Security and it's not clear if there were other similar issues within other contracts.

[joperezr]

Correct, 5 tests were disabled. And these 5 tests were the only ones failing across the whole repo with the update, so I'll assign this to you for more investigation.

[joperezr]

Are those tests the only tests that needed to be disabled during this merge?

Yes, only those 5 in the whole repo, so it might be something related to just S.N.Security

[stephentoub]

I have no idea how these tests ever passed; they seem to simply be broken:
The underlying SslState type is mocked out with a ProcessAuthentication method that's a nop:

internal void ProcessAuthentication(LazyAsyncResult lazyResult)
{
}

That means the IAsyncResult being returned from SslStream.BeginAuthenticateAsClient never completes, which is why the Task that's wrapping it never completes, and blocking on it just waits forever.

[stephentoub]

Ah, I see, LazyAsyncResult is mocked as well, and it's mocked to be completed synchronously. But in the APM pattern, even for a synchronously completed IAsyncResult, the AsyncCallback still needs to be invoked. So the mocked implementation should be fixed.

But that doesn't explain why this just started breaking... I have a theory, though. TaskFactory.FromAsync has an optimization that was added in .NET 4.5:
https://github.com/dotnet/coreclr/blob/master/src/mscorlib/src/System/Threading/Tasks/FutureFactory.cs#L839-L861
It's technically a breaking change, so it was added under a BinaryCompatibility switch. This mocked implementation would succeed with the .NET 4.5 behavior, which completes the task if the IAsyncResult returned from the Begin call completed synchronously. But this mocked implementation would fail with the .NET 4.0 behavior, which requires that the callback be invoked.

Since these started failing when we updated to a new coreclr, this suggests to me that this BinaryCompatibility switch is somehow failing and reverting us back to the .NET 4 behavior. @AlexGhiondea, @jkotas, does that sound feasible? If so, that's something we need to fix, for this and I expect other places as well.

[stephentoub]

this suggests to me that this BinaryCompatibility switch is somehow failing and reverting us back to the .NET 4 behavior

Yeah. I added a test that uses reflection to get at the properties on BinaryCompatibility and print out their values, and it prints out this:

TargetsAtLeast_Desktop_V4_5: False
TargetsAtLeast_Desktop_V4_5_1: False
AppWasBuiltForFramework: Unspecified
AppWasBuiltForVersion: 0

[stephentoub]

https://github.com/dotnet/coreclr/issues/7892