Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure cache configuration is fully completed before returning #14821

Merged

Conversation

@vbekiaris
Copy link
Contributor

vbekiaris commented Apr 2, 2019

Concurrent cache proxy creation invocations may return early, because
the cache config is visible in CacheService.configs map. However
cache configuration is not done until all subsequent config is done,
resulting in sporadic failures (eg see hazelcast/hazelcast-enterprise#2877)
due to the cache not being ready to use immediately after the cache proxy
was created.

The additional config is now wrapped in a separate method and
configs are now wrapped in futures which are only completed
once additional setup is done.

Requires EE counterpart hazelcast/hazelcast-enterprise#2883

Concurrent cache proxy creation invocations may return early, because
the cache config is visible in the configs map. However configuration
is not done until all subsequent config is done. The additional config
is now wrapped in a separate method and configs are now wrapped
in futures which are only completed once additional setup is done.
@vbekiaris vbekiaris force-pushed the vbekiaris:fixes/master/concurrent-cacheconfig-put branch from 2bad986 to fe13075 Apr 3, 2019
@vbekiaris vbekiaris marked this pull request as ready for review Apr 3, 2019
@vbekiaris

This comment has been minimized.

Copy link
Contributor Author

vbekiaris commented Apr 3, 2019

run-lab-run

@vbekiaris vbekiaris requested review from ahmetmircik and blazember Apr 3, 2019
additionalCacheConfigSetup(null, config);
} finally {
// now it is safe for others to obtain the new cache config
future.complete();

This comment has been minimized.

Copy link
@blazember

blazember Apr 4, 2019

Contributor

I'm thinking what the error case should be here. This future is meant to provide ordering between cache config initialization and its usage. Once the caller of getCacheConfig gets a config later it assumes this initialization process succeeded. I see no behavior change here since the previous EE method overridden this was running after the putIfAbsent above. But I still see some gaps here. If an exception is thrown from additionalCacheConfigSetup, we can still get an NPE later when obtaining the WAN publisher for the cache.

This comment has been minimized.

Copy link
@vbekiaris

vbekiaris Apr 4, 2019

Author Contributor

My intention was to maintain existing behavior, so even if exception is thrown from additionalCacheConfigSetup, the new CacheConfig would remain in configs map. This implementation is still safe from the NPE scenario in hazelcast/hazelcast-enterprise#2877 because every concurrently executing thread will obtain the new CacheConfig and re-attempt additionalCacheConfigSetup (in line 415 below) -> all threads waiting on putCacheConfigIfAbsent will receive the exception.

However, you are right we might be susceptible to other faults. Maybe a better option would be to remove the new CacheConfigFuture from configs map in case of exception; this is cleaner and gives user a chance to provide a correct config in case of exception. So something like:

try {
  ...
  additionalCacheConfigSetup(null, config);
  // now it is safe for others to obtain the new cache config
  future.complete();
} catch (RuntimeException e) {
  future.complete(e);
  configs.remove(cacheConfig.getNameWithPrefix(), future);
  throw e;
}

wdyt?

This comment has been minimized.

Copy link
@blazember

blazember Apr 4, 2019

Contributor

Yes, I was just thinking about removing from the map in the exception case, so 👍

I see the other callers will also receive the same exception. Missed that point.

When additional cache config fails with exception, the cache config
should be removed from the configs map so cache service is not stuck
with a half-configured cache config.
}
return localConfig;
}

protected void additionalCacheConfigSetup(CacheConfig localConfig, CacheConfig config) {

This comment has been minimized.

Copy link
@ahmetmircik

ahmetmircik Apr 5, 2019

Member

this may be clearer if we only pass config to this method?

This comment has been minimized.

Copy link
@vbekiaris

vbekiaris Apr 11, 2019

Author Contributor

Yes, modified the signature to pass just the config object (be it already existing one or new one) and a boolean indicating whether this is a new or existing config.

Copy link
Member

ahmetmircik left a comment

minor comments, seems good.

Adds javadoc to CacheConfigFuture
@vbekiaris vbekiaris merged commit a4d3d57 into hazelcast:master Apr 11, 2019
1 check passed
1 check passed
default Test PASSed.
Details
@vbekiaris

This comment has been minimized.

Copy link
Contributor Author

vbekiaris commented Apr 11, 2019

appreciate the reviews guys!

carstenartur added a commit to carstenartur/hazelcast that referenced this pull request Apr 12, 2019
…cast#14821)

Ensure cache configuration is fully completed before returning

Concurrent cache proxy creation invocations may return early, because
the cache config is visible in the configs map. However configuration
is not done until all subsequent config is done. The additional config
is now wrapped in a separate method and configs are now wrapped
in futures which are only completed once additional setup is done.
@mmedenjak mmedenjak added this to the 4.0 milestone Apr 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.