[GEODE-707] Concurrent cache-load doesn't invoke loader multiple times if there is an exception in the first load

• Type: Bug
• Status: Closed
• Priority: Major
• Resolution: Fixed
• Fix Version/s: 1.0.0-incubating.M1
• Component/s: core
• Assignee: Bruce J Schuchardt
• Reporter: Bruce J Schuchardt
• Created: 21/Dec/15 17:00
• Updated: 15/Jan/16 06:14
• Resolved: 29/Dec/15 16:02

Description

This is from an old Pivotal ticket that was fixed in bugfix branches but never made it to develop. The ticket text follows:

I see following behavior when we have a condition where 2 get() calls are sent concurrently for the same key in which Loader is called,

If the first call is sent to Loader we create a Future.
For second call we wait on this Future.
Before first call is finished we check second call its a cache miss here as well(which we have checked before) we have isCreate=true.
In case when isCreate is true in that flow we actually return null if we can't see the value(which can be true in case we got an exception on the first Loader call).

Now from customer point of view this is inconsistent, as they except GemFire? to try again with the Loader and return exception as that is the true state here.

[Darrel's reply]
The first get creates a future and when it completes sets the future. If the first get fails with an exception (like a CacheLoaderException?) it still sets the future with this code in a finally block:

VersionTag? tag = (clientEvent==null)? null : clientEvent.getVersionTag();
thisFuture.set(new Object[]

{result, tag}

);

In the case of the first get failing with an exception "result" will be null so it sets the future to an array whose first element is null.

The second get is waiting on the future. It returns this array and sees the first element as null which causes it to just return null. I think what it used to do was see Future.get return null (instead of an array with null in it) and this caused it to fall through and attempt to do the load itself which I think is what the customer also expects. You can see in the code this was what was intended by this comment that still remains:

if value == null, try our own search/load

Also if you look at the code in PartitionedRegion? that overrides this LocalRegion? code you will see that it does exactly this; if an exception is thrown the future is set to null and the second get see null returned from the future, falls through, and attempts the load itself.