Avoid NRE in cache strategies #2554

fredericDelaporte · 2020-09-20T17:40:30Z

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.

src/NHibernate/Cache/CacheFactory.cs

fredericDelaporte · 2020-09-20T17:43:58Z

src/NHibernate/Cache/NonstrictReadWriteCache.cs


 		/// <summary>
 		/// Gets the cache region name.
 		/// </summary>
 		public string RegionName
 		{
-			get { return Cache.RegionName; }
+			get { return Cache?.RegionName; }


We could use InternalCache here instead, but this seems to me good enough.

fredericDelaporte · 2020-09-20T17:50:24Z

src/NHibernate/Cache/NonstrictReadWriteCache.cs

+			if (_cache == null || _isDestroyed)
+				throw new InvalidOperationException(_isDestroyed ? "The cache has already been destroyed" : "The concrete cache is not defined");


These strategies classes are instantiated with a null underlying cache, and a later assignment on their Cache property is supplying their underlying cache. Nothing prevents this underlying cache of staying null or being set to null, without the cache strategy having been destroyed.

So we cannot just "guess" that the strategy has been destroyed if its underlying cache is null, it may be wrong.

I do not see any valid reason for the build pattern actually used by theses classes. It should likely be reworked for taking the underlying cache as a constructor parameter, mandatory, and setters should be obsoleted and removed in next major.
But that would be out of the scope of this PR.

bahusoid · 2020-09-20T18:06:03Z

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 · 2020-09-20T18:40:18Z

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 · 2020-09-20T19:17:32Z

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.

fredericDelaporte · 2020-09-20T21:28:22Z

src/NHibernate/Cache/ReadWriteCache.cs

@@ -96,6 +98,7 @@ private int NextLockId()
 		/// </remarks>
 		public object Get(CacheKey key, long txTimestamp)
 		{
+			CheckCache();
 			using (_asyncReaderWriterLock.ReadLock())


In the (strict) read-write strategy case, most cache operations acquire this lock. This class Destroy does dispose it, so most operations, in case of continued usage, would likely throw a disposed object exception from within the lock.

Somehow it is actually better than a NRE, but still not good: the strategies classes are "destroyed", so throwing a disposed object exception can be considered suitable. Excepted that it should be the strategy class throwing it directly, instead of some internal object it holds. Letting this been thrown by some internal object can puzzle end users as whether it is a bug inside NHibernate or not, like that strategy class having a bug causing usage of a disposed object.

Now it is protected by acquiring the cache prior to accessing the lock, with the cache acquisition throwing if the strategy is already destroyed.
This is a bit brittle pattern since we could acquire the cache later...

hazzik · 2020-09-22T22:52:46Z

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 · 2020-09-22T23:08:31Z

Another thought -- can CheckCache be incorporated into CacheBase?

fredericDelaporte · 2020-09-23T18:04:53Z

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 · 2020-09-23T20:35:09Z

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 · 2020-09-24T17:40:47Z

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 · 2020-09-24T20:14:57Z

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 · 2020-09-24T20:17:11Z

Can you explain why you need additional property InternalCache?

fredericDelaporte · 2020-09-25T19:20:51Z

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 · 2020-10-04T14:30:44Z

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 · 2020-10-05T08:30:29Z

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.

This reverts commit e2889f7.

fredericDelaporte · 2020-10-07T19:05:07Z

Reverted.

fredericDelaporte · 2020-10-24T15:09:11Z

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 · 2020-10-25T10:33:35Z

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 · 2020-10-26T21:15:46Z

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

mazharqayyum · 2022-01-17T14:10:16Z

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 · 2022-01-17T14:20:04Z

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

#2530 (comment)

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

Avoid NRE in cache strategies

c0532bf

fredericDelaporte added t: Bug p: Lowest Regression c: L2 Cache labels Sep 20, 2020

fredericDelaporte added this to the 5.3.4 milestone Sep 20, 2020

fredericDelaporte commented Sep 20, 2020

View reviewed changes

fredericDelaporte added 2 commits September 24, 2020 20:25

Reduce the bloat with review suggestions

996696d

Remove a double check

dc15695

fredericDelaporte added 2 commits October 4, 2020 16:12

Remove unused setter

ca5a647

Remove InternalCache

e2889f7

Revert "Remove InternalCache"

3cc8ee4

This reverts commit e2889f7.

fredericDelaporte removed this from the 5.3.4 milestone Oct 28, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid NRE in cache strategies #2554

Avoid NRE in cache strategies #2554

fredericDelaporte commented Sep 20, 2020 •

edited

Loading

fredericDelaporte Sep 20, 2020 •

edited

Loading

fredericDelaporte Sep 20, 2020 •

edited

Loading

bahusoid commented Sep 20, 2020

fredericDelaporte commented Sep 20, 2020

bahusoid commented Sep 20, 2020 •

edited

Loading

fredericDelaporte Sep 20, 2020

fredericDelaporte Sep 24, 2020

hazzik commented Sep 22, 2020

hazzik commented Sep 22, 2020 •

edited

Loading

fredericDelaporte commented Sep 23, 2020

hazzik commented Sep 23, 2020 •

edited

Loading

fredericDelaporte commented Sep 24, 2020

hazzik commented Sep 24, 2020

hazzik commented Sep 24, 2020

fredericDelaporte commented Sep 25, 2020

fredericDelaporte commented Oct 4, 2020

hazzik commented Oct 5, 2020

fredericDelaporte commented Oct 7, 2020

fredericDelaporte commented Oct 24, 2020

bahusoid commented Oct 25, 2020

hazzik commented Oct 26, 2020

mazharqayyum commented Jan 17, 2022

bahusoid commented Jan 17, 2022

		if (_cache == null \|\| _isDestroyed)
		throw new InvalidOperationException(_isDestroyed ? "The cache has already been destroyed" : "The concrete cache is not defined");

Avoid NRE in cache strategies #2554

Are you sure you want to change the base?

Avoid NRE in cache strategies #2554

Conversation

fredericDelaporte commented Sep 20, 2020 • edited Loading

fredericDelaporte Sep 20, 2020 • edited Loading

Choose a reason for hiding this comment

fredericDelaporte Sep 20, 2020 • edited Loading

Choose a reason for hiding this comment

bahusoid commented Sep 20, 2020

fredericDelaporte commented Sep 20, 2020

bahusoid commented Sep 20, 2020 • edited Loading

fredericDelaporte Sep 20, 2020

Choose a reason for hiding this comment

fredericDelaporte Sep 24, 2020

Choose a reason for hiding this comment

hazzik commented Sep 22, 2020

hazzik commented Sep 22, 2020 • edited Loading

fredericDelaporte commented Sep 23, 2020

hazzik commented Sep 23, 2020 • edited Loading

fredericDelaporte commented Sep 24, 2020

hazzik commented Sep 24, 2020

hazzik commented Sep 24, 2020

fredericDelaporte commented Sep 25, 2020

fredericDelaporte commented Oct 4, 2020

hazzik commented Oct 5, 2020

fredericDelaporte commented Oct 7, 2020

fredericDelaporte commented Oct 24, 2020

bahusoid commented Oct 25, 2020

hazzik commented Oct 26, 2020

mazharqayyum commented Jan 17, 2022

bahusoid commented Jan 17, 2022

fredericDelaporte commented Sep 20, 2020 •

edited

Loading

fredericDelaporte Sep 20, 2020 •

edited

Loading

fredericDelaporte Sep 20, 2020 •

edited

Loading

bahusoid commented Sep 20, 2020 •

edited

Loading

hazzik commented Sep 22, 2020 •

edited

Loading

hazzik commented Sep 23, 2020 •

edited

Loading