keith-turner commented on code in PR #50:
URL:
https://github.com/apache/accumulo-classloaders/pull/50#discussion_r2709986985
##########
modules/local-caching-classloader/src/main/java/org/apache/accumulo/classloader/lcc/LocalCachingContextClassLoaderFactory.java:
##########
@@ -235,69 +265,77 @@ private static boolean
cacheKeyMatchesContextLocation(String cacheKey, String co
}
private void checkMonitoredLocation(String contextLocation, long interval) {
- ContextDefinition currentDef =
- contextDefs.compute(contextLocation, (contextLocationKey,
previousDefinition) -> {
- if (previousDefinition == null) {
- return null;
- }
- // check for any classloaders still in the cache that were created
for a context
- // definition found at this URL
- if (!classloaders
- .anyMatch(cacheKey -> cacheKeyMatchesContextLocation(cacheKey,
contextLocation))) {
- LOG.debug("ClassLoader for context {} not present, no longer
monitoring for changes",
- contextLocation);
- return null;
- }
- return previousDefinition;
- });
- if (currentDef == null) {
- // context has been removed from the map, no need to check for update
- LOG.debug("ContextDefinition for context {} not present, no longer
monitoring for changes",
- contextLocation);
- return;
- }
- long nextInterval = interval;
+ cleanupLock.readLock().lock();
try {
- final ContextDefinition update = getDefinition(contextLocation);
- if (!currentDef.getChecksum().equals(update.getChecksum())) {
- LOG.debug("Context definition for {} has changed", contextLocation);
- localStore.get().storeContextResources(update);
- contextDefs.put(contextLocation, update);
- nextInterval = update.getMonitorIntervalSeconds();
- classloaderFailures.remove(contextLocation);
- } else {
- LOG.trace("Context definition for {} has not changed",
contextLocation);
- }
- } catch (IOException | RuntimeException e) {
- LOG.error("Error parsing updated context definition at {}. Classloader
NOT updated!",
- contextLocation, e);
- final Stopwatch failureTimer = classloaderFailures.get(contextLocation);
- if (updateFailureGracePeriodMins.isZero()) {
- // failure monitoring is disabled
- LOG.debug("Property {} not set, not tracking classloader failures for
context {}",
- UPDATE_FAILURE_GRACE_PERIOD_MINS, contextLocation);
- } else if (failureTimer == null) {
- // first failure, start the timer
- classloaderFailures.put(contextLocation, Stopwatch.createStarted());
- LOG.debug(
- "Tracking classloader failures for context {}, will NOT return
working classloader if failures continue for {} minutes",
- contextLocation, updateFailureGracePeriodMins.toMinutes());
- } else if
(failureTimer.elapsed().compareTo(updateFailureGracePeriodMins) > 0) {
- // has been failing for the grace period
- // unset the classloader reference so that the failure
- // will return from getClassLoader in the calling thread
- LOG.info("Grace period for failing classloader has elapsed for context
{}",
+ ContextDefinition currentDef =
+ contextDefs.compute(contextLocation, (contextLocationKey,
previousDefinition) -> {
+ if (previousDefinition == null) {
+ return null;
+ }
+ // check for any classloaders still in the cache that were created
for a context
+ // definition found at this URL
+ if (!classloaders
+ .anyMatch(cacheKey -> cacheKeyMatchesContextLocation(cacheKey,
contextLocation))) {
+ LOG.debug("ClassLoader for context {} not present, no longer
monitoring for changes",
+ contextLocation);
+ return null;
+ }
+ return previousDefinition;
+ });
+ if (currentDef == null) {
+ // context has been removed from the map, no need to check for update
+ LOG.debug("ContextDefinition for context {} not present, no longer
monitoring for changes",
contextLocation);
- contextDefs.remove(contextLocation);
- classloaderFailures.remove(contextLocation);
- } else {
- LOG.trace("Failing to update classloader for context {} within the
grace period",
+ return;
+ }
+ long nextInterval = interval;
+ try {
+ final ContextDefinition update = getDefinition(contextLocation);
+ if (!currentDef.getChecksum().equals(update.getChecksum())) {
+ LOG.debug("Context definition for {} has changed", contextLocation);
+ localStore.get().storeContextResources(update);
+ contextDefs.put(contextLocation, update);
+ nextInterval = update.getMonitorIntervalSeconds();
+ classloaderFailures.remove(contextLocation);
+ } else {
+ LOG.trace("Context definition for {} has not changed",
contextLocation);
+ }
+ // reschedule this task to run
+ monitorContext(contextLocation, nextInterval);
Review Comment:
This would be a bit safer as outlined in another comment. Then all three
places that reschedule will do so w/ consistent locking.
```suggestion
// Atomically lock on the context key and only reschedule if the
context is present.
contextDefs.compute(contextLocation, (k,v)->{
if(v != null){
monitorContext(contextLocation, nextInterval);
}
return v;
});
```
##########
modules/local-caching-classloader/src/main/java/org/apache/accumulo/classloader/lcc/LocalCachingContextClassLoaderFactory.java:
##########
@@ -235,69 +265,77 @@ private static boolean
cacheKeyMatchesContextLocation(String cacheKey, String co
}
private void checkMonitoredLocation(String contextLocation, long interval) {
- ContextDefinition currentDef =
- contextDefs.compute(contextLocation, (contextLocationKey,
previousDefinition) -> {
- if (previousDefinition == null) {
- return null;
- }
- // check for any classloaders still in the cache that were created
for a context
- // definition found at this URL
- if (!classloaders
- .anyMatch(cacheKey -> cacheKeyMatchesContextLocation(cacheKey,
contextLocation))) {
- LOG.debug("ClassLoader for context {} not present, no longer
monitoring for changes",
- contextLocation);
- return null;
- }
- return previousDefinition;
- });
- if (currentDef == null) {
- // context has been removed from the map, no need to check for update
- LOG.debug("ContextDefinition for context {} not present, no longer
monitoring for changes",
- contextLocation);
- return;
- }
- long nextInterval = interval;
+ cleanupLock.readLock().lock();
try {
- final ContextDefinition update = getDefinition(contextLocation);
- if (!currentDef.getChecksum().equals(update.getChecksum())) {
- LOG.debug("Context definition for {} has changed", contextLocation);
- localStore.get().storeContextResources(update);
- contextDefs.put(contextLocation, update);
- nextInterval = update.getMonitorIntervalSeconds();
- classloaderFailures.remove(contextLocation);
- } else {
- LOG.trace("Context definition for {} has not changed",
contextLocation);
- }
- } catch (IOException | RuntimeException e) {
- LOG.error("Error parsing updated context definition at {}. Classloader
NOT updated!",
- contextLocation, e);
- final Stopwatch failureTimer = classloaderFailures.get(contextLocation);
- if (updateFailureGracePeriodMins.isZero()) {
- // failure monitoring is disabled
- LOG.debug("Property {} not set, not tracking classloader failures for
context {}",
- UPDATE_FAILURE_GRACE_PERIOD_MINS, contextLocation);
- } else if (failureTimer == null) {
- // first failure, start the timer
- classloaderFailures.put(contextLocation, Stopwatch.createStarted());
- LOG.debug(
- "Tracking classloader failures for context {}, will NOT return
working classloader if failures continue for {} minutes",
- contextLocation, updateFailureGracePeriodMins.toMinutes());
- } else if
(failureTimer.elapsed().compareTo(updateFailureGracePeriodMins) > 0) {
- // has been failing for the grace period
- // unset the classloader reference so that the failure
- // will return from getClassLoader in the calling thread
- LOG.info("Grace period for failing classloader has elapsed for context
{}",
+ ContextDefinition currentDef =
+ contextDefs.compute(contextLocation, (contextLocationKey,
previousDefinition) -> {
+ if (previousDefinition == null) {
+ return null;
+ }
+ // check for any classloaders still in the cache that were created
for a context
+ // definition found at this URL
+ if (!classloaders
+ .anyMatch(cacheKey -> cacheKeyMatchesContextLocation(cacheKey,
contextLocation))) {
+ LOG.debug("ClassLoader for context {} not present, no longer
monitoring for changes",
+ contextLocation);
+ return null;
+ }
+ return previousDefinition;
+ });
+ if (currentDef == null) {
+ // context has been removed from the map, no need to check for update
+ LOG.debug("ContextDefinition for context {} not present, no longer
monitoring for changes",
contextLocation);
- contextDefs.remove(contextLocation);
- classloaderFailures.remove(contextLocation);
- } else {
- LOG.trace("Failing to update classloader for context {} within the
grace period",
+ return;
+ }
+ long nextInterval = interval;
+ try {
+ final ContextDefinition update = getDefinition(contextLocation);
+ if (!currentDef.getChecksum().equals(update.getChecksum())) {
+ LOG.debug("Context definition for {} has changed", contextLocation);
+ localStore.get().storeContextResources(update);
+ contextDefs.put(contextLocation, update);
+ nextInterval = update.getMonitorIntervalSeconds();
+ classloaderFailures.remove(contextLocation);
+ } else {
+ LOG.trace("Context definition for {} has not changed",
contextLocation);
+ }
+ // reschedule this task to run
+ monitorContext(contextLocation, nextInterval);
+ } catch (IOException | RuntimeException e) {
+ LOG.error("Error parsing updated context definition at {}. Classloader
NOT updated!",
contextLocation, e);
+ final Stopwatch failureTimer =
classloaderFailures.get(contextLocation);
+ if (updateFailureGracePeriodMins.isZero()) {
+ // failure monitoring is disabled
+ LOG.debug("Property {} not set, not tracking classloader failures
for context {}",
+ UPDATE_FAILURE_GRACE_PERIOD_MINS, contextLocation);
+ } else if (failureTimer == null) {
+ // first failure, start the timer
+ classloaderFailures.put(contextLocation, Stopwatch.createStarted());
+ LOG.debug(
+ "Tracking classloader failures for context {}, will NOT return
working classloader if failures continue for {} minutes",
+ contextLocation, updateFailureGracePeriodMins.toMinutes());
+ } else if
(failureTimer.elapsed().compareTo(updateFailureGracePeriodMins) > 0) {
+ // has been failing for the grace period
+ // unset the classloader reference so that the failure
+ // will return from getClassLoader in the calling thread
+ LOG.info("Grace period for failing classloader has elapsed for
context {}",
+ contextLocation);
+ contextDefs.remove(contextLocation);
+ classloaderFailures.remove(contextLocation);
+ } else {
+ LOG.trace("Failing to update classloader for context {} within the
grace period",
+ contextLocation, e);
+ }
+ // reschedule this task to run
+ // Don't put this in finally block as we only want to reschedule
+ // on success or handled exception
+ monitorContext(contextLocation, nextInterval);
Review Comment:
A bit further up in this code we remove the context location from
contextDefs. In this case we do not want to reschedule. The code seems to
assume that if there is a key/value present in the context defs map that there
exists a background thread. A really general way to fix this may be do
something like the following. This makes rescheduling always atomic per key
avoiding any race conditions. There could be other cases that remove the map
location and this defends against that. This also aligns the background thread
w/ the foreground thread which also schedules in the compute method (although
it does it in the case where v==null).
```
// Atomically lock on the context key and only reschedule if the
context is present.
contextDefs.compute(contextLocation, (k,v)->{
if(v != null){
monitorContext(contextLocation, nextInterval);
}
return v;
});
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
