Avoid NRE in cache strategies

[fredericDelaporte]

This fixes a regression from #1480, where destroying the externally supplied and shared cache has been replaced by setting it to null.

Related: #2530.

But this change causes further usages of the cache to cause null reference exceptions, instead of meaningful errors.

This PR is in my opinion the most correct minimal fix.

Another option could be to just remove the line setting the cache to null, effectively causing Destroy to be no-op. But this would mean letting the user use "destroyed" caches, which seems a bit wrong. And in the case of read-write caches, other failures would occur due to some internal object being disposed.

[bahusoid]

TBH I don't like this change. All these checks just to throw not NRE but different kind of exception feels like not useful change that unnecessarily clutters the code.

So I would either go with:

just remove the line setting the cache to null, effectively causing Destroy to be no-op

Or maybe introduce some kind of "Null Object" cache and assign it instead. Which either simply makes no op on calls (it seems we already have one - FakeCache) or throws InvalidOperationException.

[fredericDelaporte]

And what about the read-write case, which will fail on its disposed lock? It is no better than a NRE and we should also handle this case more gracefully, don't you think?

[bahusoid]

And what about the read-write case

Sorry I don't know this part of code - so have no idea what would fail on its disposed lock and in which case (Empty Destroy method or with "Null Value Object" cache or in both cases). So implementing FakeCache like class that throws user friendly exception and use it instead of null won't work? Ok.. Then I have no better suggestions..

It is no better than a NRE and we should also handle this case more gracefully, don't you think?

I am for graceful behavior if it's possible to be implemented in one place. I don't like scattering such checks across all implementations. If it's the only choice - I think it's not worth it for the rare cases when object is used for disposed session factory.

So that's just my thoughts I'm not requesting you to make any changes (sorry forgot to nit :) ). Let it be reviewed by someone else.

[hazzik]

Would it be better if CheckCache is encapsulated into Cache property and the usage of _cache field is updated with the usage of property?

[hazzik]

Another thought -- can CheckCache be incorporated into CacheBase?

[fredericDelaporte]

Yes I dislike too this bloat code for checking the object state everywhere.

But there is also the _asyncReaderWriterLock which can be disposed and so needs to be protected by those checks. Maybe we can use your first suggestion and dodge the lock trouble by nullifying it and using ?. every where on it to dodge the issue and let the later Cache access raise the exception, but that would be a code smell I think, those ?. on the lock.

I will use your suggestion for the two other caches at least.

About incorporating CheckCache in CacheBase, I do not get it right now, I will have another look.

[hazzik]

incorporating CheckCache in CacheBase, I do not get it right now, I will have another look.

Now most of our calls look like this:

// prop definition simplified
CacheBase Cache => _cache;

void SomeMethod() {
    CheckCache();
    _cache.DoSomething();
...
}

What I'm proposing is doing this:

// prop definition simplified
CacheBase Cache { get { CheckCache(); return _cache; } }

void SomeMethod() {
    Cache.DoSomething();
...
}

[fredericDelaporte]

I thought you wanted me to put something in the CacheBase class, but instead you refer to the CacheBase typed property versus the ICache one. One trouble is, currently, the CacheBase one is an explicit interface implementation and so is unpractical to directly call. The ICache one is on the path to removal.

Maybe we should dodge that by adding an InternalCache property, with a todo 6.0 move code into CacheBase Cache and remove property.

It would also avoid adding a possible breaking change in a minor by starting to throw on read of those interface properties for null caches.

[hazzik]

I thought you wanted me to put something in the CacheBase class, but instead you refer to the CacheBase typed property versus the ICache one.

There were 2 suggestions actually. One is about CacheBase class and another one about properties (both properties in fact).

[hazzik]

Can you explain why you need additional property InternalCache?

[fredericDelaporte]

As commented before doing it:

• To avoid a possible breaking change in a SemVer patch release. Changing one of the existing property for throwing when the cache is null, instead of just yielding it, is a possible breaking change since theses properties are public.
• Doing it in the CacheBase typed property would require some cumbersome, bloat code everywhere to ensure we call it, since it is an explicit implementation. Something like IBatchableCacheConcurrencyStrategy strategy = this; var cache = strategy.Cache;.

[fredericDelaporte]

I have added a commit to remove InternalCache property. But as explained, it adds some other kind of bloat code, in order to be able to call the explicitly implemented CacheBase property. The ICache property is not suitable because ICache does not have all the required methods. By example we cannot call GetMany on an ICache typed property.

[hazzik]

I have added a commit to remove InternalCache property.

I have not had anything against this property. I was just wondering why it is needed. The explanation and reasoning was fine.

[fredericDelaporte]

Reverted.

[fredericDelaporte]

This PR is holding the release of 5.3.4 since ten days (latest merge done on the 5.3.x branch). It should be validated or rejected, and then I would prepare the release of the 5.3.4 fixes.

[bahusoid]

It should be validated or rejected

I still think the issue is not worth changes made in this PR. And it shouldn't hold the release - this PR can wait till next round as NRE or other exception it's about user invalid usage case scenario.

[hazzik]

I lean on agreeing with @bahusoid on the issue. We probably should release the 5.3.4 without this change.

[mazharqayyum]

It should be validated or rejected

I still think the issue is not worth changes made in this PR. And it shouldn't hold the release - this PR can wait till next round as NRE or other exception it's about user invalid usage case scenario.

Can you please expand on invalid usage scenario? ISQL DDL query causes this exception, I have the L2 SysCache configured. Its a known behavior of Nhibernate that any direct update to database will wipe the L2 cache. It seems like a good deal for really rare scenario which in my case is. So what is the recommended way in 5.3.x?

[bahusoid]

This PR just changes one exception with another. It doesn't fix anything. So it won't help you

Can you please expand on invalid usage scenario

https://github.com/nhibernate/nhibernate-core/issues/2530#issuecomment-691234620

So if you can prove that it's reproducible with still open session factory (check ISessionFactory.IsClosed) we should investigate it more closely.