Remove IsAvailable check on CommitAsync.

[adityamandaleeka]

Fix #42009

I did some archaeology into how we ended up adding this check. I think (but am not certain yet) this was added because one of the incremental states of #26098 had a null-forgiving operator on the IdBytes getter. IdBytes could return null if IsAvailable wasn't set.

This led to the addition of the if (!IsAvailable) check in Serialize which was subsequently moved to CommitAsync based on this comment.

However, the IdBytes getter was changed to make its result non-null:

aspnetcore/src/Middleware/Session/src/DistributedSession.cs

Lines 119 to 131 in bf02bf2

	private byte[] IdBytes
	{
	get
	{
	Load();
	if (_sessionIdBytes == null)
	{
	_sessionIdBytes = new byte[IdByteCount];
	RandomNumberGenerator.Fill(_sessionIdBytes);
	}
	return _sessionIdBytes;
	}
	}

Assuming this was the concern that led to the IsAvailable check being added to CommitAsync, I think we're okay to remove it. I'd like another pair of eyes on this though.

[BrennanConroy]

// use field to avoid loading the session via IsAvailable property
if (!_isAvailable)

?

[adityamandaleeka]

Not sure we want that; if _isAvailable wasn't set, it could be that we never attempted to load it. The current code attempts to load in that case.

[halter73]

Why would we remove this check in CommitAsync but not in Set?

[adityamandaleeka]

Set always depends on IsAvailable, but CommitAsync might not need to load the session if _isModified isn't true. The IsAvailable check was added in CommitAsync during the nullability work but I don't see why it's necessary now.

[halter73]

I see. And it could force sync-over-async as well. Yuck!

[csturm83]

@adityamandaleeka Will this regression fix be back ported to .NET 6 (LTS) ?

[ghost]

Hi @csturm83. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

[adityamandaleeka]

@csturm83 It doesn't seem severe enough for a 6.0 backport, but we could consider it if there's a compelling use case that's broken.

[csturm83]

@adityamandaleeka Between the unexpected/undocumented behavior change (exiting early vs. throwing an exception), consistency/symmetry with LoadAsync exceptional behavior, performance hit of potential extra deserialization, synch over async, etc. - in my opinion, it seems worth it.

That, and it's a relatively focused change that restores documented behavior and expectations (addresses a regression).

[ghost]

Hi @csturm83. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

[Tratcher]

@csturm83 backporting is expensive and risky, a bug must have high impact and no viable workaround. While in-efficient, it's not clear that this bug is causing a blocking issue for you?

[Alex-Covecube]

Just wanted to let you guys know that the IsAvailable call you're discussing was causing massive CPU load on our servers.

Scenario:

We originally used MongoDB for our IDistributedCache implementation, and that worked well for a while when our traffic was low. As our server load grew we started moving our code to Redis with the expectation of reducing overhead.

Imagine our surprise when Redis actually consumed an order of magnitude more CPU on the web servers than the previous MongoDB implementation. We used the StackExchange Redis driver with Microsoft.Extensions.Caching.StackExchangeRedis for the ASP.NET Core integration, really standard stuff.

To make a long story short, after much analysis it turns out that having IsAvailable in CommitAsync (DistributedSession class) forces the Redis driver to do a sync get on every page load, and apparently StackExchange Redis is not that efficient with sync calls.

Solutions tried:

• Increase the number of threads for the socket thread pool on StackExchange Redis in the hopes of making enough threads available for more sync calls.
• Completely replace Microsoft.Extensions.Caching.StackExchangeRedis with a custom IDistributedCache.
• Completely reimplement the entire DistributedSession : ISession, and remove the IsAvailable call in CommitAsync.

Only the last option worked well enough to actually decrease load on the web servers to approximately what we saw with the older MongoDB implementation.

To me, it seems like this issue can cause a massive performance drop under pretty common circumstances. Especially given that the official docs recommend the exact setup that we had (https://learn.microsoft.com/en-us/aspnet/core/performance/caching/distributed)

[ghost]

Hi @Alex-Covecube. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

[csturm83]

@Tratcher see @Alex-Covecube 's comment above (wasn't sure if comments on closed issues generate notification without an @ mention). Silent performance traps that can spike CPU usage could be costly.

For those of us in enterprise tied to LTS releases, it would be a shame to have to wait until 8.0 for this regression fix.

[ghost]

Hi @csturm83. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

[adityamandaleeka]

@Alex-Covecube thanks for letting us know. I've reopened the issue for backport consideration.