only cache credentials on success

[wfurt]

This is not what I expected. But I could reproduce the Safe handle has been closed error with few hundred runs.
With this change I did ~ 5000 runs without single failure. There may be more to it but this definitely seems to help.

I have some ideas why we do not see this more:

runtime/src/libraries/System.Net.Security/src/System/Net/Security/SslSessionsCache.cs

Lines 196 to 209 in f3390fd

	static void ShrinkCredentialCache()
	{
	//
	// A simplest way of preventing infinite cache grows.
	//
	// Security relief (DoS):
	// A number of active creds is never greater than a number of _outstanding_
	// security sessions, i.e. SSL connections.
	// So we will try to shrink cache to the number of active creds once in a while.
	//
	// We won't shrink cache in the case when NO new handles are coming to it.
	//
	if ((s_cachedCreds.Count % CheckExpiredModulo) == 0)

With this we would try to string the cache only on multiples of 32. With certificate, protocols and EncryptionPolicy that generally would be never on clients as there is typically no client certificate, few protocol variations and nobody sane should not use anything but Encrypt policy.

So it tales some effort to even hit the reduction code - perhaps by crafting many unique certificates.
For future investigations, lowering CheckExpiredModulo makes it much easier to test that code.

While we lock on cache changes, we don't seems to when we retrieving cached value.
And all the entries stored in the cache are disposed.
And we seems to allocate a lot while attempting to shrink:

runtime/src/libraries/System.Net.Security/src/System/Net/Security/SslSessionsCache.cs

Lines 224 to 225 in f3390fd

	cahced.Dispose();
	cahced = SafeCredentialReference.CreateReference(creds);

So while the caching may be improved, I did not find anything particularly wrong.
My current speculation is that the observed issue is caused by polluting cache with bad values.
With the added extra check my local runs are now happy.

Fixes #30724
Fixes #46770

[ghost]

Tagging subscribers to this area: @dotnet/ncl, @vcsjones
See info in area-owners.md if you want to be subscribed.

Issue Details

---

This is not what I expected. But I could reproduce the Safe handle has been closed error with few hundred runs.
With this change I did ~ 5000 runs without single failure. There may be more to it but this definitely seems to help.

I have some ideas why we do not see this more:

runtime/src/libraries/System.Net.Security/src/System/Net/Security/SslSessionsCache.cs

Lines 196 to 209 in f3390fd

	static void ShrinkCredentialCache()
	{
	//
	// A simplest way of preventing infinite cache grows.
	//
	// Security relief (DoS):
	// A number of active creds is never greater than a number of _outstanding_
	// security sessions, i.e. SSL connections.
	// So we will try to shrink cache to the number of active creds once in a while.
	//
	// We won't shrink cache in the case when NO new handles are coming to it.
	//
	if ((s_cachedCreds.Count % CheckExpiredModulo) == 0)

With this we would try to string the cache only on multiples of 32. With certificate, protocols and EncryptionPolicy that generally would be never on clients as there is typically no client certificate, few protocol variations and nobody sane should not use anything but Encrypt policy.

So it tales some effort to even hit the reduction code - perhaps by crafting many unique certificates.
For future investigations, lowering CheckExpiredModulo makes it much easier to test that code.

While we lock on cache changes, we don't seems to when we retrieving cached value.
And all the entries stored in the cache are disposed.
And we seems to allocate a lot while attempting to shrink:

runtime/src/libraries/System.Net.Security/src/System/Net/Security/SslSessionsCache.cs

Lines 224 to 225 in f3390fd

	cahced.Dispose();
	cahced = SafeCredentialReference.CreateReference(creds);

So while the caching may be improved, I did not find anything particularly wrong.
My current speculation is that the observed issue is caused by polluting cache with bad values.
With the added extra check my local runs are now happy.

fixes #30724

Author:	wfurt
Assignees:	wfurt
Labels:	area-System.Net.Security
Milestone:	-

[karelz]

I assume it fixes also #46770 -- top post edited.

[wfurt]

I assume it fixes also #46770 -- top post edited.

Yes, I think it should. I was not assigned to me so I miss it.

[stephentoub]

Do we have tests that will end up with a non-OK ErrorCode?

[wfurt]

I will take a look. There seems to be some race condition so I see the "Close handle" only once in a while. It would be nice to have some predictability for error cases.