Socket: don't perform RST close on Dispose when user called Shutdown

[tmds]

Fixes https://github.com/dotnet/corefx/issues/41189
Regressed in #38804.

CC @halter73 @davidsh @wfurt @stephentoub @jkotalik

[stephentoub]

I'm having trouble following what abortive vs canAbort means. Can you clarify?

[tmds]

Before this PR, TryUnblockSocket always performed an abortive (that is :Connection reset by peer) close.
We're changing to take into account if the user has called Socket.Shutdown.
canAbort tells TryUnblockSocket if it can do an abortive close.

[stephentoub]

Before this PR, TryUnblockSocket always performed an abortive close.

But TryUnblockSocket was already taking a bool abortive argument. If it always performed an abortive close, what was the purpose of that argument?

[tmds]

There is a special case for Sockets that don't have the CLOEXEC flag set:

corefx/src/System.Net.Sockets/src/System/Net/Sockets/SafeSocketHandle.Unix.cs

Lines 379 to 381 in 485441f

	// Unless we're doing an abortive close, don't touch sockets which don't have the CLOEXEC flag set.
	// These may be shared with other processes and we want to avoid disconnecting them.
	if (!abortive)

[stephentoub]

I'm still struggling here, @tmds. Are you saying that both canAbort and abortive mean "do an abortive close", but they communicate that information to different parts of the stack? The names are too close to be meaningful.

[tmds]

I missed this comment.

canAbort contains whether a CloseAsIs that was called with abortive: false, can become a an innerSocket.Close with abortive: true.

[tmds]

I'll remove canAbort and replace it with !hasShutdownSend.

[davidsh]

@stephentoub Do you have any additional feedback on this? Or are you ready to sign-off?

[davidsh]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 4 pipeline(s).

[halter73]

It might be worth asserting in TryUnblockSocket that canAbort is always true if abortive is true. The names are a little confusing. Maybe a tri-state enum would be better, but that's definitely a nit.

[tmds]

There are a number of System.Security.Cryptography.Cng test failures, like:

Internal.Cryptography.CryptoThrowHelper+WindowsCryptographicException : The parameter is incorrect.

Stack trace
   at System.Security.Cryptography.CngKey.SetProperty(CngProperty property) in /_/src/System.Security.Cryptography.Cng/src/System/Security/Cryptography/CngKey.Properties.cs:line 72
   at Internal.Cryptography.BasicSymmetricCipherNCrypt..ctor(Func`1 cngKeyFactory, CipherMode cipherMode, Int32 blockSizeInBytes, Byte[] iv, Boolean encrypting) in /_/src/System.Security.Cryptography.Cng/src/Internal/Cryptography/BasicSymmetricCipherNCrypt.cs:line 36
   at Internal.Cryptography.CngSymmetricAlgorithmCore.CreatePersistedCryptoTransformCore(Func`1 cngKeyFactory, Byte[] iv, Boolean encrypting) in /_/src/System.Security.Cryptography.Cng/src/Internal/Cryptography/CngSymmetricAlgorithmCore.cs:line 181
   at Internal.Cryptography.CngSymmetricAlgorithmCore.CreateCryptoTransform(Boolean encrypting) in /_/src/System.Security.Cryptography.Cng/src/Internal/Cryptography/CngSymmetricAlgorithmCore.cs:line 130
   at Internal.Cryptography.CngSymmetricAlgorithmCore.CreateEncryptor() in /_/src/System.Security.Cryptography.Cng/src/Internal/Cryptography/CngSymmetricAlgorithmCore.cs:line 105
   at System.Security.Cryptography.TripleDESCng.CreateEncryptor() in /_/src/System.Security.Cryptography.Cng/src/System/Security/Cryptography/TripleDESCng.cs:line 79
   at System.Security.Cryptography.Cng.Tests.SymmetricCngTestHelpers.VerifyPersistedKey(String keyName, Int32 plainBytesCount, Func`2 persistedFunc, Func`1 ephemeralFunc, CipherMode cipherMode, PaddingMode paddingMode) in /_/src/System.Security.Cryptography.Cng/tests/SymmetricCngTestHelpers.cs:line 74
   at System.Security.Cryptography.Cng.Tests.SymmetricCngTestHelpers.VerifyPersistedKey(CngAlgorithm algorithm, Int32 keySize, Int32 plainBytesCount, Func`2 persistedFunc, Func`1 ephemeralFunc, CipherMode cipherMode, PaddingMode paddingMode) in /_/src/System.Security.Cryptography.Cng/tests/SymmetricCngTestHelpers.cs:line 39
   at System.Security.Cryptography.Cng.Tests.TripleDESCngTests.VerifyPersistedKey(Int32 plainBytesCount, CipherMode cipherMode, PaddingMode paddingMode) in /_/src/System.Security.Cryptography.Cng/tests/TripleDESCngTests.cs:line 30

There is also a failure of PostAsyncDuplex_CancelledBeforeResponseHeadersReceived_ResetsStream.

Error message
Assert.Throws() Failure\r\nExpected: typeof(System.OperationCanceledException)\r\nActual: (No exception was thrown)

Stack trace
   at System.Net.Http.Functional.Tests.HttpClientHandlerTest_Http2.PostAsyncDuplex_CancelledBeforeResponseHeadersReceived_ResetsStream() in /_/src/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Http2.cs:line 2304
--- End of stack trace from previous location where exception was thrown ---

Is this good to merge?

[stephentoub]

Is this good to merge?

I was hoping to continue to discuss #41250 (comment)

[tmds]

@stephentoub I removed the canAbort local.

CI passed except for Azure DevOps unavailable:

TF10216: Azure DevOps services are currently unavailable.

[stephentoub]

Thanks, @tmds.

[stephentoub]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 4 pipeline(s).