code review for catalog CLI
Project: http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/repo Commit: http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/commit/d6ae0511 Tree: http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/tree/d6ae0511 Diff: http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/diff/d6ae0511 Branch: refs/heads/master Commit: d6ae0511c35ef4070c8a1637bad1ef3b113bda81 Parents: eef7891 Author: Alex Heneveld <[email protected]> Authored: Mon May 18 16:02:07 2015 +0100 Committer: Alex Heneveld <[email protected]> Committed: Wed May 20 00:39:50 2015 +0100 ---------------------------------------------------------------------- .../brooklyn/entity/rebind/RebindManager.java | 7 +- .../mementos/BrooklynMementoPersister.java | 1 + .../catalog/internal/BasicBrooklynCatalog.java | 43 +++--- .../brooklyn/catalog/internal/CatalogDo.java | 1 + .../catalog/internal/CatalogInitialization.java | 139 ++++++++++--------- .../rebind/PeriodicDeltaChangeListener.java | 37 +++-- .../brooklyn/entity/rebind/RebindIteration.java | 5 +- .../entity/rebind/RebindManagerImpl.java | 10 +- .../BrooklynMementoPersisterToObjectStore.java | 2 +- .../internal/AbstractManagementContext.java | 16 ++- .../NonDeploymentManagementContext.java | 7 +- .../catalog/internal/CatalogScanTest.java | 1 + .../entity/rebind/ActivePartialRebindTest.java | 2 +- .../brooklyn/entity/rebind/RebindTestUtils.java | 2 +- ...ntoPersisterInMemorySizeIntegrationTest.java | 21 ++- .../osgi/OsgiVersionMoreEntityTest.java | 4 +- usage/cli/src/main/java/brooklyn/cli/Main.java | 3 +- .../brooklyn/launcher/BrooklynLauncher.java | 6 +- .../brooklyn/rest/resources/ServerResource.java | 9 +- .../brooklyn/rest/HaMasterCheckFilterTest.java | 2 +- .../util/javalang/AggregateClassLoader.java | 41 +++++- 21 files changed, 205 insertions(+), 154 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/d6ae0511/api/src/main/java/brooklyn/entity/rebind/RebindManager.java ---------------------------------------------------------------------- diff --git a/api/src/main/java/brooklyn/entity/rebind/RebindManager.java b/api/src/main/java/brooklyn/entity/rebind/RebindManager.java index 501430d..8e4cb29 100644 --- a/api/src/main/java/brooklyn/entity/rebind/RebindManager.java +++ b/api/src/main/java/brooklyn/entity/rebind/RebindManager.java @@ -20,7 +20,6 @@ package brooklyn.entity.rebind; import java.util.List; import java.util.Map; -import java.util.concurrent.TimeUnit; import java.util.concurrent.TimeoutException; import javax.annotation.Nullable; @@ -105,13 +104,9 @@ public interface RebindManager { * waiting for activity there to cease (interrupting in the case of {@link #stopReadOnly()}). */ public void stop(); - /** @deprecated since 0.7.0; use {@link #waitForPendingComplete(Duration)} */ @VisibleForTesting - @Deprecated - public void waitForPendingComplete(long timeout, TimeUnit unit) throws InterruptedException, TimeoutException; /** waits for any needed or pending writes to complete */ - @VisibleForTesting - public void waitForPendingComplete(Duration duration) throws InterruptedException, TimeoutException; + public void waitForPendingComplete(Duration duration, boolean canTrigger) throws InterruptedException, TimeoutException; /** Forcibly performs persistence, in the foreground * @deprecated since 0.7.0; use {@link #forcePersistNow(boolean, PersistenceExceptionHandler)}, * default parameter here is false to mean incremental, with null/default exception handler */ http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/d6ae0511/api/src/main/java/brooklyn/mementos/BrooklynMementoPersister.java ---------------------------------------------------------------------- diff --git a/api/src/main/java/brooklyn/mementos/BrooklynMementoPersister.java b/api/src/main/java/brooklyn/mementos/BrooklynMementoPersister.java index 33f1de7..ec16e9b 100644 --- a/api/src/main/java/brooklyn/mementos/BrooklynMementoPersister.java +++ b/api/src/main/java/brooklyn/mementos/BrooklynMementoPersister.java @@ -103,6 +103,7 @@ public interface BrooklynMementoPersister { /** applies a partial write of state delta */ void delta(Delta delta, PersistenceExceptionHandler exceptionHandler); /** inserts an additional delta to be written on the next delta request */ + @Beta void queueDelta(Delta delta); void enableWriteAccess(); http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/d6ae0511/core/src/main/java/brooklyn/catalog/internal/BasicBrooklynCatalog.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/brooklyn/catalog/internal/BasicBrooklynCatalog.java b/core/src/main/java/brooklyn/catalog/internal/BasicBrooklynCatalog.java index 7e4763d..88e5818 100644 --- a/core/src/main/java/brooklyn/catalog/internal/BasicBrooklynCatalog.java +++ b/core/src/main/java/brooklyn/catalog/internal/BasicBrooklynCatalog.java @@ -621,9 +621,9 @@ public class BasicBrooklynCatalog implements BrooklynCatalog { } else { // scan for annotations: if libraries here, scan them; if inherited libraries error; else scan classpath if (!libraryBundlesNew.isEmpty()) { - result.addAll(scanAnnotations(mgmt, libraryBundlesNew)); + result.addAll(scanAnnotationsFromBundles(mgmt, libraryBundlesNew)); } else if (libraryBundles.isEmpty()) { - result.addAll(scanAnnotations(mgmt, null)); + result.addAll(scanAnnotationsFromLocal(mgmt)); } else { throw new IllegalStateException("Cannot scan catalog node no local bundles, and with inherited bundles we will not scan the classpath"); } @@ -780,25 +780,26 @@ public class BasicBrooklynCatalog implements BrooklynCatalog { return oldValue; } - /** scans the given libraries for annotated items, or if null scans the local classpath */ - private Collection<CatalogItemDtoAbstract<?, ?>> scanAnnotations(ManagementContext mgmt, Collection<CatalogBundle> libraries) { -// CatalogDto dto = CatalogDto.newDefaultLocalScanningDto(CatalogClasspathDo.CatalogScanningModes.ANNOTATIONS); - CatalogDto dto; + private Collection<CatalogItemDtoAbstract<?, ?>> scanAnnotationsFromLocal(ManagementContext mgmt) { + CatalogDto dto = CatalogDto.newNamedInstance("Local Scanned Catalog", "All annotated Brooklyn entities detected in the classpath", "scanning-local-classpath"); + return scanAnnotationsInternal(mgmt, new CatalogDo(dto)); + } + + private Collection<CatalogItemDtoAbstract<?, ?>> scanAnnotationsFromBundles(ManagementContext mgmt, Collection<CatalogBundle> libraries) { String[] urls = null; - if (libraries==null) { - dto = CatalogDto.newNamedInstance("Local Scanned Catalog", "All annotated Brooklyn entities detected in the classpath", "scanning-local-classpath"); - } else { - dto = CatalogDto.newNamedInstance("Bundles Scanned Catalog", "All annotated Brooklyn entities detected in the classpath", "scanning-bundles-classpath-"+libraries.hashCode()); - urls = new String[libraries.size()]; - int i=0; - for (CatalogBundle b: libraries) - urls[i++] = b.getUrl(); - } + CatalogDto dto = CatalogDto.newNamedInstance("Bundles Scanned Catalog", "All annotated Brooklyn entities detected in the classpath", "scanning-bundles-classpath-"+libraries.hashCode()); + urls = new String[libraries.size()]; + int i=0; + for (CatalogBundle b: libraries) + urls[i++] = b.getUrl(); + CatalogDo subCatalog = new CatalogDo(dto); + subCatalog.addToClasspath(urls); + return scanAnnotationsInternal(mgmt, subCatalog); + } + + private Collection<CatalogItemDtoAbstract<?, ?>> scanAnnotationsInternal(ManagementContext mgmt, CatalogDo subCatalog) { subCatalog.mgmt = mgmt; - if (urls!=null) { - subCatalog.addToClasspath(urls); - } // else use local classpath subCatalog.setClasspathScanForEntities(CatalogScanningModes.ANNOTATIONS); subCatalog.load(); // TODO apply metadata? (extract YAML from the items returned) @@ -1045,7 +1046,7 @@ public class BasicBrooklynCatalog implements BrooklynCatalog { } private CatalogItem<?,?> addItemDto(CatalogItemDtoAbstract<?, ?> itemDto, boolean forceUpdate) { - CatalogItem<?, ?> existingDto = checkItemIsDuplicateOrDisallowed(itemDto, true, forceUpdate); + CatalogItem<?, ?> existingDto = checkItemAllowedAndIfSoReturnAnyDuplicate(itemDto, true, forceUpdate); if (existingDto!=null) { // it's a duplicate, and not forced, just return it log.trace("Using existing duplicate for catalog item {}", itemDto.getId()); @@ -1072,9 +1073,9 @@ public class BasicBrooklynCatalog implements BrooklynCatalog { return itemDto; } - /** returns item DTO if item is an allowed duplicate, null if it should be added, or false if the item is an allowed duplicate, + /** returns item DTO if item is an allowed duplicate, or null if it should be added (there is no duplicate), * throwing if item cannot be added */ - private CatalogItem<?, ?> checkItemIsDuplicateOrDisallowed(CatalogItem<?,?> itemDto, boolean allowDuplicates, boolean forceUpdate) { + private CatalogItem<?, ?> checkItemAllowedAndIfSoReturnAnyDuplicate(CatalogItem<?,?> itemDto, boolean allowDuplicates, boolean forceUpdate) { if (forceUpdate) return null; CatalogItemDo<?, ?> existingItem = getCatalogItemDo(itemDto.getSymbolicName(), itemDto.getVersion()); if (existingItem == null) return null; http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/d6ae0511/core/src/main/java/brooklyn/catalog/internal/CatalogDo.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/brooklyn/catalog/internal/CatalogDo.java b/core/src/main/java/brooklyn/catalog/internal/CatalogDo.java index 2142ce9..de68213 100644 --- a/core/src/main/java/brooklyn/catalog/internal/CatalogDo.java +++ b/core/src/main/java/brooklyn/catalog/internal/CatalogDo.java @@ -267,6 +267,7 @@ public class CatalogDo { return loadCatalog(child); } + /** adds the given urls; filters out any nulls supplied */ public synchronized void addToClasspath(String ...urls) { if (dto.classpath == null) dto.classpath = new CatalogClasspathDto(); http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/d6ae0511/core/src/main/java/brooklyn/catalog/internal/CatalogInitialization.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/brooklyn/catalog/internal/CatalogInitialization.java b/core/src/main/java/brooklyn/catalog/internal/CatalogInitialization.java index ded7dc4..6f0b80e 100644 --- a/core/src/main/java/brooklyn/catalog/internal/CatalogInitialization.java +++ b/core/src/main/java/brooklyn/catalog/internal/CatalogInitialization.java @@ -37,7 +37,7 @@ import brooklyn.util.exceptions.FatalRuntimeException; import brooklyn.util.exceptions.RuntimeInterruptedException; import brooklyn.util.flags.TypeCoercions; import brooklyn.util.guava.Maybe; -import brooklyn.util.net.Urls; +import brooklyn.util.os.Os; import brooklyn.util.text.Strings; import com.google.common.annotations.Beta; @@ -53,7 +53,7 @@ public class CatalogInitialization implements ManagementContextInjectable { A1) if not persisting, go to B1 A2) if --catalog-reset, delete persisted catalog items - A3) read persisted catalog items (possibly deleted in A2), go to C1 + A3) if there is a persisted catalog (and it wasn't not deleted by A2), read it and go to C1 A4) go to B1 B1) look for --catalog-initial, if so read it, then go to C1 @@ -71,20 +71,20 @@ public class CatalogInitialization implements ManagementContextInjectable { private static final Logger log = LoggerFactory.getLogger(CatalogInitialization.class); - String initialUri; - boolean reset; - String additionsUri; - boolean force; - - boolean disallowLocal = false; - List<Function<CatalogInitialization, Void>> callbacks = MutableList.of(); - boolean hasRunBestEffort = false, hasRunOfficial = false, isPopulating = false; + private String initialUri; + private boolean reset; + private String additionsUri; + private boolean force; + + private boolean disallowLocal = false; + private List<Function<CatalogInitialization, Void>> callbacks = MutableList.of(); + private boolean hasRunBestEffort = false, hasRunOfficial = false, isPopulating = false; - ManagementContext managementContext; - boolean isStartingUp = false; - boolean failOnStartupErrors = false; + private ManagementContext managementContext; + private boolean isStartingUp = false; + private boolean failOnStartupErrors = false; - Object mutex = new Object(); + private Object populatingCatalogMutex = new Object(); public CatalogInitialization(String initialUri, boolean reset, String additionUri, boolean force) { this.initialUri = initialUri; @@ -98,19 +98,30 @@ public class CatalogInitialization implements ManagementContextInjectable { } public void injectManagementContext(ManagementContext managementContext) { - if (this.managementContext!=null && managementContext!=null && !this.managementContext.equals(managementContext)) - throw new IllegalStateException("Cannot switch management context of "+this+"; from "+this.managementContext+" to "+managementContext); + Preconditions.checkNotNull(managementContext, "management context"); + if (this.managementContext!=null && managementContext!=this.managementContext) + throw new IllegalStateException("Cannot switch management context, from "+this.managementContext+" to "+managementContext); this.managementContext = managementContext; } - public ManagementContext getManagementContext() { - return Preconditions.checkNotNull(managementContext, "management context has not been injected into "+this); + /** Called by the framework to set true while starting up, and false afterwards, + * in order to assist in appropriate logging and error handling. */ + public void setStartingUp(boolean isStartingUp) { + this.isStartingUp = isStartingUp; + } + + public void setFailOnStartupErrors(boolean startupFailOnCatalogErrors) { + this.failOnStartupErrors = startupFailOnCatalogErrors; } public CatalogInitialization addPopulationCallback(Function<CatalogInitialization, Void> callback) { callbacks.add(callback); return this; } + + public ManagementContext getManagementContext() { + return Preconditions.checkNotNull(managementContext, "management context has not been injected into "+this); + } public boolean isInitialResetRequested() { return reset; @@ -121,9 +132,9 @@ public class CatalogInitialization implements ManagementContextInjectable { /** makes or updates the mgmt catalog, based on the settings in this class */ public void populateCatalog(boolean needsInitial, Collection<CatalogItem<?, ?>> optionalItemsForResettingCatalog) { - try { - isPopulating = true; - synchronized (mutex) { + synchronized (populatingCatalogMutex) { + try { + isPopulating = true; BasicBrooklynCatalog catalog = (BasicBrooklynCatalog) managementContext.getCatalog(); if (!catalog.getCatalog().isLoaded()) { catalog.load(); @@ -136,15 +147,15 @@ public class CatalogInitialization implements ManagementContextInjectable { } hasRunOfficial = true; - populateCatalog(catalog, needsInitial, true, optionalItemsForResettingCatalog); + populateCatalogImpl(catalog, needsInitial, optionalItemsForResettingCatalog); + } finally { + hasRunOfficial = true; + isPopulating = false; } - } finally { - hasRunOfficial = true; - isPopulating = false; } } - private void populateCatalog(BasicBrooklynCatalog catalog, boolean needsInitial, boolean runCallbacks, Collection<CatalogItem<?, ?>> optionalItemsForResettingCatalog) { + private void populateCatalogImpl(BasicBrooklynCatalog catalog, boolean needsInitial, Collection<CatalogItem<?, ?>> optionalItemsForResettingCatalog) { applyCatalogLoadMode(); if (optionalItemsForResettingCatalog!=null) { @@ -157,9 +168,7 @@ public class CatalogInitialization implements ManagementContextInjectable { populateAdditions(catalog); - if (runCallbacks) { - populateViaCallbacks(catalog); - } + populateViaCallbacks(catalog); } private enum PopulateMode { YAML, XML, AUTODETECT } @@ -190,13 +199,13 @@ public class CatalogInitialization implements ManagementContextInjectable { return; } - catalogUrl = Urls.mergePaths(BrooklynServerConfig.getMgmtBaseDir( managementContext.getConfig() ), "catalog.bom"); + catalogUrl = Os.mergePaths(BrooklynServerConfig.getMgmtBaseDir( managementContext.getConfig() ), "catalog.bom"); if (new File(catalogUrl).exists()) { populateInitialFromUri(catalog, "file:"+catalogUrl, PopulateMode.YAML); return; } - catalogUrl = Urls.mergePaths(BrooklynServerConfig.getMgmtBaseDir( managementContext.getConfig() ), "catalog.xml"); + catalogUrl = Os.mergePaths(BrooklynServerConfig.getMgmtBaseDir( managementContext.getConfig() ), "catalog.xml"); if (new File(catalogUrl).exists()) { populateInitialFromUri(catalog, "file:"+catalogUrl, PopulateMode.XML); return; @@ -244,7 +253,14 @@ public class CatalogInitialization implements ManagementContextInjectable { if (result==null && contents!=null && (mode==PopulateMode.XML || mode==PopulateMode.AUTODETECT)) { // then try XML - problem = populateInitialFromUriXml(catalog, catalogUrl, problem, contents); + try { + populateInitialFromUriXml(catalog, catalogUrl, contents); + // clear YAML problem + problem = null; + } catch (Exception e) { + Exceptions.propagateIfFatal(e); + if (problem==null) problem = e; + } } if (result!=null) { @@ -259,19 +275,11 @@ public class CatalogInitialization implements ManagementContextInjectable { // deprecated XML format @SuppressWarnings("deprecation") - private Exception populateInitialFromUriXml(BasicBrooklynCatalog catalog, String catalogUrl, Exception problem, String contents) { - CatalogDto dto = null; - try { - dto = CatalogDto.newDtoFromXmlContents(contents, catalogUrl); - problem = null; - } catch (Exception e) { - Exceptions.propagateIfFatal(e); - if (problem==null) problem = e; - } + private void populateInitialFromUriXml(BasicBrooklynCatalog catalog, String catalogUrl, String contents) { + CatalogDto dto = CatalogDto.newDtoFromXmlContents(contents, catalogUrl); if (dto!=null) { catalog.reset(dto); } - return problem; } boolean hasRunAdditions = false; @@ -303,6 +311,7 @@ public class CatalogInitialization implements ManagementContextInjectable { callback.apply(this); } + private Object setFromCLMMutex = new Object(); private boolean setFromCatalogLoadMode = false; /** @deprecated since introduced in 0.7.0, only for legacy compatibility with @@ -310,29 +319,31 @@ public class CatalogInitialization implements ManagementContextInjectable { * allowing control of catalog loading from a brooklyn property */ @Deprecated public void applyCatalogLoadMode() { - if (setFromCatalogLoadMode) return; - setFromCatalogLoadMode = true; - Maybe<Object> clmm = ((ManagementContextInternal)managementContext).getConfig().getConfigRaw(BrooklynServerConfig.CATALOG_LOAD_MODE, false); - if (clmm.isAbsent()) return; - brooklyn.catalog.CatalogLoadMode clm = TypeCoercions.coerce(clmm.get(), brooklyn.catalog.CatalogLoadMode.class); - log.warn("Legacy CatalogLoadMode "+clm+" set: applying, but this should be changed to use new CLI --catalogXxx commands"); - switch (clm) { - case LOAD_BROOKLYN_CATALOG_URL: - reset = true; - break; - case LOAD_BROOKLYN_CATALOG_URL_IF_NO_PERSISTED_STATE: - // now the default - break; - case LOAD_PERSISTED_STATE: - disallowLocal = true; - break; + synchronized (setFromCLMMutex) { + if (setFromCatalogLoadMode) return; + setFromCatalogLoadMode = true; + Maybe<Object> clmm = ((ManagementContextInternal)managementContext).getConfig().getConfigRaw(BrooklynServerConfig.CATALOG_LOAD_MODE, false); + if (clmm.isAbsent()) return; + brooklyn.catalog.CatalogLoadMode clm = TypeCoercions.coerce(clmm.get(), brooklyn.catalog.CatalogLoadMode.class); + log.warn("Legacy CatalogLoadMode "+clm+" set: applying, but this should be changed to use new CLI --catalogXxx commands"); + switch (clm) { + case LOAD_BROOKLYN_CATALOG_URL: + reset = true; + break; + case LOAD_BROOKLYN_CATALOG_URL_IF_NO_PERSISTED_STATE: + // now the default + break; + case LOAD_PERSISTED_STATE: + disallowLocal = true; + break; + } } } /** makes the catalog, warning if persistence is on and hasn't run yet * (as the catalog will be subsequently replaced) */ public void populateBestEffort(BasicBrooklynCatalog catalog) { - synchronized (mutex) { + synchronized (populatingCatalogMutex) { if (hasRunOfficial || hasRunBestEffort || isPopulating) return; // if a thread calls back in to this, ie calling to it from a getCatalog() call while populating, // it will own the mutex and observe isRunningBestEffort, returning quickly @@ -341,7 +352,7 @@ public class CatalogInitialization implements ManagementContextInjectable { if (isStartingUp) { log.warn("Catalog access requested when not yet initialized; populating best effort rather than through recommended pathway. Catalog data may be replaced subsequently."); } - populateCatalog(catalog, true, true, null); + populateCatalogImpl(catalog, true, null); } finally { hasRunBestEffort = true; isPopulating = false; @@ -349,14 +360,6 @@ public class CatalogInitialization implements ManagementContextInjectable { } } - public void setStartingUp(boolean isStartingUp) { - this.isStartingUp = isStartingUp; - } - - public void setFailOnStartupErrors(boolean startupFailOnCatalogErrors) { - this.failOnStartupErrors = startupFailOnCatalogErrors; - } - public void handleException(Throwable throwable, Object details) { if (throwable instanceof InterruptedException) throw new RuntimeInterruptedException((InterruptedException) throwable); http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/d6ae0511/core/src/main/java/brooklyn/entity/rebind/PeriodicDeltaChangeListener.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/brooklyn/entity/rebind/PeriodicDeltaChangeListener.java b/core/src/main/java/brooklyn/entity/rebind/PeriodicDeltaChangeListener.java index cd33f01..f3861bc 100644 --- a/core/src/main/java/brooklyn/entity/rebind/PeriodicDeltaChangeListener.java +++ b/core/src/main/java/brooklyn/entity/rebind/PeriodicDeltaChangeListener.java @@ -25,6 +25,7 @@ import java.util.concurrent.Callable; import java.util.concurrent.Semaphore; import java.util.concurrent.TimeUnit; import java.util.concurrent.TimeoutException; +import java.util.concurrent.atomic.AtomicInteger; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -177,6 +178,7 @@ public class PeriodicDeltaChangeListener implements ChangeListener { private final Semaphore persistingMutex = new Semaphore(1); private final Object startStopMutex = new Object(); + private final AtomicInteger writeCount = new AtomicInteger(0); private PersistenceActivityMetrics metrics; @@ -230,7 +232,7 @@ public class PeriodicDeltaChangeListener implements ChangeListener { CountdownTimer expiry = timeout.countdownTimer(); try { scheduledTask.cancel(false); - waitForPendingComplete(expiry.getDurationRemaining().lowerBound(Duration.ZERO).add(graceTimeoutForSubsequentOperations)); + waitForPendingComplete(expiry.getDurationRemaining().lowerBound(Duration.ZERO).add(graceTimeoutForSubsequentOperations), true); } catch (Exception e) { throw Exceptions.propagate(e); } @@ -243,7 +245,6 @@ public class PeriodicDeltaChangeListener implements ChangeListener { scheduledTask = null; } - // Discard all state that was waiting to be persisted synchronized (this) { deltaCollector = new DeltaCollector(); @@ -255,30 +256,38 @@ public class PeriodicDeltaChangeListener implements ChangeListener { } } - /** - * @deprecated since 0.7.0, use {@link #waitForPendingComplete(Duration)} - */ - @VisibleForTesting - public void waitForPendingComplete(long timeout, TimeUnit unit) throws InterruptedException, TimeoutException { - waitForPendingComplete(Duration.of(timeout, unit)); - } /** Waits for any in-progress writes to be completed then for or any unwritten data to be written. */ @VisibleForTesting - public void waitForPendingComplete(Duration timeout) throws InterruptedException, TimeoutException { + public void waitForPendingComplete(Duration timeout, boolean canTrigger) throws InterruptedException, TimeoutException { if (!isActive() && !stopping) return; CountdownTimer timer = timeout.isPositive() ? CountdownTimer.newInstanceStarted(timeout) : CountdownTimer.newInstancePaused(Duration.PRACTICALLY_FOREVER); + Integer targetWriteCount = null; // wait for mutex, so we aren't tricked by an in-progress who has already recycled the collector if (persistingMutex.tryAcquire(timer.getDurationRemaining().toMilliseconds(), TimeUnit.MILLISECONDS)) { try { // now no one else is writing if (!deltaCollector.isEmpty()) { - // but there is data that needs to be written - persistNowSafely(true); + if (canTrigger) { + // but there is data that needs to be written + persistNowSafely(true); + } else { + targetWriteCount = writeCount.get()+1; + } } } finally { persistingMutex.release(); } + if (targetWriteCount!=null) { + while (writeCount.get() <= targetWriteCount) { + Duration left = timer.getDurationRemaining(); + if (left.isPositive()) { + writeCount.wait(left.lowerBound(Duration.millis(10)).toMilliseconds()); + } else { + throw new TimeoutException("Timeout waiting for independent write of rebind-periodic-delta, after "+timer.getDurationElapsed()); + } + } + } } else { // someone else has been writing for the entire time throw new TimeoutException("Timeout waiting for completion of in-progress write of rebind-periodic-delta, after "+timer.getDurationElapsed()); @@ -425,6 +434,10 @@ public class PeriodicDeltaChangeListener implements ChangeListener { LOG.debug("Problem persisting, but no longer active (ignoring)", e); } } finally { + synchronized (writeCount) { + writeCount.incrementAndGet(); + writeCount.notifyAll(); + } if (!alreadyHasMutex) persistingMutex.release(); } } http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/d6ae0511/core/src/main/java/brooklyn/entity/rebind/RebindIteration.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/brooklyn/entity/rebind/RebindIteration.java b/core/src/main/java/brooklyn/entity/rebind/RebindIteration.java index 6124a54..2d1e47f 100644 --- a/core/src/main/java/brooklyn/entity/rebind/RebindIteration.java +++ b/core/src/main/java/brooklyn/entity/rebind/RebindIteration.java @@ -345,7 +345,6 @@ public abstract class RebindIteration { Collection<CatalogItem<?, ?>> catalogItems = rebindContext.getCatalogItems(); CatalogInitialization catInit = ((ManagementContextInternal)managementContext).getCatalogInitialization(); - catInit.injectManagementContext(managementContext); catInit.applyCatalogLoadMode(); Collection<CatalogItem<?,?>> itemsForResettingCatalog = null; boolean needsInitialCatalog; @@ -365,9 +364,7 @@ public abstract class RebindIteration { itemsForResettingCatalog = MutableList.<CatalogItem<?,?>>of(); PersisterDeltaImpl delta = new PersisterDeltaImpl(); - for (String catalogItemId: mementoRawData.getCatalogItems().keySet()) { - delta.removedCatalogItemIds.add(catalogItemId); - } + delta.removedCatalogItemIds.addAll(mementoRawData.getCatalogItems().keySet()); getPersister().queueDelta(delta); mementoRawData.clearCatalogItems(); http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/d6ae0511/core/src/main/java/brooklyn/entity/rebind/RebindManagerImpl.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/brooklyn/entity/rebind/RebindManagerImpl.java b/core/src/main/java/brooklyn/entity/rebind/RebindManagerImpl.java index caf04c5..ded0049 100644 --- a/core/src/main/java/brooklyn/entity/rebind/RebindManagerImpl.java +++ b/core/src/main/java/brooklyn/entity/rebind/RebindManagerImpl.java @@ -421,15 +421,9 @@ public class RebindManagerImpl implements RebindManager { @Override @VisibleForTesting - @Deprecated /** @deprecated since 0.7.0 use Duration as argument */ - public void waitForPendingComplete(long timeout, TimeUnit unit) throws InterruptedException, TimeoutException { - waitForPendingComplete(Duration.of(timeout, unit)); - } - @Override - @VisibleForTesting - public void waitForPendingComplete(Duration timeout) throws InterruptedException, TimeoutException { + public void waitForPendingComplete(Duration timeout, boolean canTrigger) throws InterruptedException, TimeoutException { if (persistenceStoreAccess == null || !persistenceRunning) return; - persistenceRealChangeListener.waitForPendingComplete(timeout); + persistenceRealChangeListener.waitForPendingComplete(timeout, canTrigger); persistenceStoreAccess.waitForWritesCompleted(timeout); } @Override http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/d6ae0511/core/src/main/java/brooklyn/entity/rebind/persister/BrooklynMementoPersisterToObjectStore.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/brooklyn/entity/rebind/persister/BrooklynMementoPersisterToObjectStore.java b/core/src/main/java/brooklyn/entity/rebind/persister/BrooklynMementoPersisterToObjectStore.java index 47eefdc..23e40e9 100644 --- a/core/src/main/java/brooklyn/entity/rebind/persister/BrooklynMementoPersisterToObjectStore.java +++ b/core/src/main/java/brooklyn/entity/rebind/persister/BrooklynMementoPersisterToObjectStore.java @@ -565,7 +565,7 @@ public class BrooklynMementoPersisterToObjectStore implements BrooklynMementoPer while (!queuedDeltas.isEmpty()) { Delta extraDelta = queuedDeltas.remove(0); - doDelta(extraDelta, exceptionHandler, false); + doDelta(extraDelta, exceptionHandler, true); } doDelta(delta, exceptionHandler, false); http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/d6ae0511/core/src/main/java/brooklyn/management/internal/AbstractManagementContext.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/brooklyn/management/internal/AbstractManagementContext.java b/core/src/main/java/brooklyn/management/internal/AbstractManagementContext.java index 1cbe312..b1fdc0c 100644 --- a/core/src/main/java/brooklyn/management/internal/AbstractManagementContext.java +++ b/core/src/main/java/brooklyn/management/internal/AbstractManagementContext.java @@ -82,6 +82,7 @@ import brooklyn.util.task.Tasks; import com.google.common.base.Function; import com.google.common.base.Objects; +import com.google.common.base.Preconditions; public abstract class AbstractManagementContext implements ManagementContextInternal { private static final Logger log = LoggerFactory.getLogger(AbstractManagementContext.class); @@ -441,10 +442,10 @@ public abstract class AbstractManagementContext implements ManagementContextInte return uri; } + private Object catalogInitMutex = new Object(); @Override public CatalogInitialization getCatalogInitialization() { - if (catalogInitialization!=null) return catalogInitialization; - synchronized (this) { + synchronized (catalogInitMutex) { if (catalogInitialization!=null) return catalogInitialization; CatalogInitialization ci = new CatalogInitialization(); setCatalogInitialization(ci); @@ -453,9 +454,14 @@ public abstract class AbstractManagementContext implements ManagementContextInte } @Override - public synchronized void setCatalogInitialization(CatalogInitialization catalogInitialization) { - if (catalogInitialization!=null) catalogInitialization.injectManagementContext(this); - this.catalogInitialization = catalogInitialization; + public void setCatalogInitialization(CatalogInitialization catalogInitialization) { + synchronized (catalogInitMutex) { + Preconditions.checkNotNull(catalogInitialization, "initialization must not be null"); + if (this.catalogInitialization!=null && this.catalogInitialization != catalogInitialization) + throw new IllegalStateException("Changing catalog init from "+this.catalogInitialization+" to "+catalogInitialization+"; changes not permitted"); + catalogInitialization.injectManagementContext(this); + this.catalogInitialization = catalogInitialization; + } } public BrooklynObject lookup(String id) { http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/d6ae0511/core/src/main/java/brooklyn/management/internal/NonDeploymentManagementContext.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/brooklyn/management/internal/NonDeploymentManagementContext.java b/core/src/main/java/brooklyn/management/internal/NonDeploymentManagementContext.java index 00639a5..b437561 100644 --- a/core/src/main/java/brooklyn/management/internal/NonDeploymentManagementContext.java +++ b/core/src/main/java/brooklyn/management/internal/NonDeploymentManagementContext.java @@ -27,7 +27,6 @@ import java.util.Collections; import java.util.List; import java.util.Map; import java.util.concurrent.ExecutionException; -import java.util.concurrent.TimeUnit; import java.util.concurrent.TimeoutException; import org.slf4j.Logger; @@ -544,11 +543,7 @@ public class NonDeploymentManagementContext implements ManagementContextInternal } @Override - public void waitForPendingComplete(long timeout, TimeUnit unit) throws InterruptedException, TimeoutException { - throw new IllegalStateException("Non-deployment context "+NonDeploymentManagementContext.this+" is not valid for this operation."); - } - @Override - public void waitForPendingComplete(Duration timeout) throws InterruptedException, TimeoutException { + public void waitForPendingComplete(Duration timeout, boolean canTrigger) throws InterruptedException, TimeoutException { throw new IllegalStateException("Non-deployment context "+NonDeploymentManagementContext.this+" is not valid for this operation."); } @Override http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/d6ae0511/core/src/test/java/brooklyn/catalog/internal/CatalogScanTest.java ---------------------------------------------------------------------- diff --git a/core/src/test/java/brooklyn/catalog/internal/CatalogScanTest.java b/core/src/test/java/brooklyn/catalog/internal/CatalogScanTest.java index 07c40af..5b32bed 100644 --- a/core/src/test/java/brooklyn/catalog/internal/CatalogScanTest.java +++ b/core/src/test/java/brooklyn/catalog/internal/CatalogScanTest.java @@ -169,6 +169,7 @@ public class CatalogScanTest { int numFromAnnots = Iterables.size(annotsCatalog.getCatalogItems(Predicates.alwaysTrue())); Assert.assertEquals(numInDefault, numFromAnnots); + Assert.assertTrue(numInDefault>0, "Expected more than 0 entries"); } // a simple test asserting no errors when listing the real catalog, and listing them for reference http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/d6ae0511/core/src/test/java/brooklyn/entity/rebind/ActivePartialRebindTest.java ---------------------------------------------------------------------- diff --git a/core/src/test/java/brooklyn/entity/rebind/ActivePartialRebindTest.java b/core/src/test/java/brooklyn/entity/rebind/ActivePartialRebindTest.java index f43cbe5..afeca2b 100644 --- a/core/src/test/java/brooklyn/entity/rebind/ActivePartialRebindTest.java +++ b/core/src/test/java/brooklyn/entity/rebind/ActivePartialRebindTest.java @@ -83,7 +83,7 @@ public class ActivePartialRebindTest extends RebindTestFixtureWithApp { public void testRebindCheckingMemoryLeak() throws Exception { TestEntity c1 = origApp.addChild(EntitySpec.create(TestEntity.class)); Entities.manage(c1); - c1.setConfig(TestEntity.CONF_NAME, Strings.makeRandomId(1000000)); + c1.config().set(TestEntity.CONF_NAME, Strings.makeRandomId(1000000)); gcAndLog("before"); long used0 = Runtime.getRuntime().totalMemory() - Runtime.getRuntime().freeMemory(); http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/d6ae0511/core/src/test/java/brooklyn/entity/rebind/RebindTestUtils.java ---------------------------------------------------------------------- diff --git a/core/src/test/java/brooklyn/entity/rebind/RebindTestUtils.java b/core/src/test/java/brooklyn/entity/rebind/RebindTestUtils.java index 939d42e..ad0d407 100644 --- a/core/src/test/java/brooklyn/entity/rebind/RebindTestUtils.java +++ b/core/src/test/java/brooklyn/entity/rebind/RebindTestUtils.java @@ -431,7 +431,7 @@ public class RebindTestUtils { } public static void waitForPersisted(ManagementContext managementContext) throws InterruptedException, TimeoutException { - managementContext.getRebindManager().waitForPendingComplete(TIMEOUT); + managementContext.getRebindManager().waitForPendingComplete(TIMEOUT, true); } public static void checkCurrentMementoSerializable(Application app) throws Exception { http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/d6ae0511/core/src/test/java/brooklyn/entity/rebind/persister/BrooklynMementoPersisterInMemorySizeIntegrationTest.java ---------------------------------------------------------------------- diff --git a/core/src/test/java/brooklyn/entity/rebind/persister/BrooklynMementoPersisterInMemorySizeIntegrationTest.java b/core/src/test/java/brooklyn/entity/rebind/persister/BrooklynMementoPersisterInMemorySizeIntegrationTest.java index d2638cf..ac647c6 100644 --- a/core/src/test/java/brooklyn/entity/rebind/persister/BrooklynMementoPersisterInMemorySizeIntegrationTest.java +++ b/core/src/test/java/brooklyn/entity/rebind/persister/BrooklynMementoPersisterInMemorySizeIntegrationTest.java @@ -47,22 +47,29 @@ public class BrooklynMementoPersisterInMemorySizeIntegrationTest extends Brookly } public void testPersistenceVolumeFast() throws IOException, TimeoutException, InterruptedException { - doTestPersistenceVolume(50*1000, false); + doTestPersistenceVolume(50*1000, false, true); } @Test(groups="Integration",invocationCount=20) public void testPersistenceVolumeFastManyTimes() throws IOException, TimeoutException, InterruptedException { - doTestPersistenceVolume(50*1000, false); + doTestPersistenceVolume(50*1000, false, true); } @Test(groups="Integration") public void testPersistenceVolumeWaiting() throws IOException, TimeoutException, InterruptedException { // by waiting we ensure there aren't extra writes going on - doTestPersistenceVolume(50*1000, true); + doTestPersistenceVolume(50*1000, true, true); + } + public void testPersistenceVolumeFastNoTrigger() throws IOException, TimeoutException, InterruptedException { + doTestPersistenceVolume(50*1000, false, false); + } + @Test(groups="Integration",invocationCount=20) + public void testPersistenceVolumeFastNoTriggerManyTimes() throws IOException, TimeoutException, InterruptedException { + doTestPersistenceVolume(50*1000, false, false); } - protected void doTestPersistenceVolume(int bigBlockSize, boolean forceDelay) throws IOException, TimeoutException, InterruptedException { + protected void doTestPersistenceVolume(int bigBlockSize, boolean forceDelay, boolean canTrigger) throws IOException, TimeoutException, InterruptedException { if (forceDelay) Time.sleep(Duration.FIVE_SECONDS); else recorder.blockUntilDataWrittenExceeds(512, Duration.FIVE_SECONDS); - localManagementContext.getRebindManager().waitForPendingComplete(Duration.FIVE_SECONDS); + localManagementContext.getRebindManager().waitForPendingComplete(Duration.FIVE_SECONDS, canTrigger); long out1 = recorder.getBytesOut(); int filesOut1 = recorder.getCountDataOut(); @@ -73,7 +80,7 @@ public class BrooklynMementoPersisterInMemorySizeIntegrationTest extends Brookly ((EntityInternal)app).setAttribute(TestEntity.NAME, "hello world"); if (forceDelay) Time.sleep(Duration.FIVE_SECONDS); else recorder.blockUntilDataWrittenExceeds(out1+10, Duration.FIVE_SECONDS); - localManagementContext.getRebindManager().waitForPendingComplete(Duration.FIVE_SECONDS); + localManagementContext.getRebindManager().waitForPendingComplete(Duration.FIVE_SECONDS, canTrigger); long out2 = recorder.getBytesOut(); Assert.assertTrue(out2-out1>10, "should have written more data"); @@ -86,7 +93,7 @@ public class BrooklynMementoPersisterInMemorySizeIntegrationTest extends Brookly ((EntityInternal)entity).setAttribute(TestEntity.NAME, Identifiers.makeRandomId(bigBlockSize)); if (forceDelay) Time.sleep(Duration.FIVE_SECONDS); else recorder.blockUntilDataWrittenExceeds(out2+bigBlockSize, Duration.FIVE_SECONDS); - localManagementContext.getRebindManager().waitForPendingComplete(Duration.FIVE_SECONDS); + localManagementContext.getRebindManager().waitForPendingComplete(Duration.FIVE_SECONDS, canTrigger); long out3 = recorder.getBytesOut(); Assert.assertTrue(out3-out2 > bigBlockSize, "should have written 50k more data, only wrote "+out3+" compared with "+out2); http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/d6ae0511/core/src/test/java/brooklyn/management/osgi/OsgiVersionMoreEntityTest.java ---------------------------------------------------------------------- diff --git a/core/src/test/java/brooklyn/management/osgi/OsgiVersionMoreEntityTest.java b/core/src/test/java/brooklyn/management/osgi/OsgiVersionMoreEntityTest.java index 967bb6d..7f4009d 100644 --- a/core/src/test/java/brooklyn/management/osgi/OsgiVersionMoreEntityTest.java +++ b/core/src/test/java/brooklyn/management/osgi/OsgiVersionMoreEntityTest.java @@ -56,6 +56,7 @@ import brooklyn.util.guava.Maybe; import brooklyn.util.os.Os; import brooklyn.util.osgi.Osgis; +import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableMap; import com.google.common.collect.Iterables; @@ -142,7 +143,8 @@ public class OsgiVersionMoreEntityTest { static CatalogItem<?, ?> addCatalogItemWithNameAndType(ManagementContext mgmt, String symName, String version, String type, String ...libraries) { CatalogEntityItemDto c1 = newCatalogItemWithNameAndType(symName, version, type, libraries); mgmt.getCatalog().addItem(c1); - CatalogItem<?, ?> c2 = mgmt.getCatalog().getCatalogItem(type, version); + CatalogItem<?, ?> c2 = mgmt.getCatalog().getCatalogItem(symName, version); + Preconditions.checkNotNull(c2, "Item "+type+":"+version+" was not found after adding it"); return c2; } http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/d6ae0511/usage/cli/src/main/java/brooklyn/cli/Main.java ---------------------------------------------------------------------- diff --git a/usage/cli/src/main/java/brooklyn/cli/Main.java b/usage/cli/src/main/java/brooklyn/cli/Main.java index e6330ea..d98e0d8 100644 --- a/usage/cli/src/main/java/brooklyn/cli/Main.java +++ b/usage/cli/src/main/java/brooklyn/cli/Main.java @@ -286,8 +286,7 @@ public class Main extends AbstractMain { public boolean startBrooklynNode = false; // Note in some cases, you can get java.util.concurrent.RejectedExecutionException - // if shutdown is not co-ordinated, e.g. with Whirr: - // looks like: {@linktourl https://gist.github.com/47066f72d6f6f79b953e} + // if shutdown is not co-ordinated, looks like: {@linktourl https://gist.github.com/47066f72d6f6f79b953e} @Beta @Option(name = { "-sk", "--stopOnKeyPress" }, description = "Shutdown immediately on user text entry after startup (useful for debugging and demos)") http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/d6ae0511/usage/launcher/src/main/java/brooklyn/launcher/BrooklynLauncher.java ---------------------------------------------------------------------- diff --git a/usage/launcher/src/main/java/brooklyn/launcher/BrooklynLauncher.java b/usage/launcher/src/main/java/brooklyn/launcher/BrooklynLauncher.java index 3a87087..6a201a2 100644 --- a/usage/launcher/src/main/java/brooklyn/launcher/BrooklynLauncher.java +++ b/usage/launcher/src/main/java/brooklyn/launcher/BrooklynLauncher.java @@ -676,7 +676,9 @@ public class BrooklynLauncher { brooklynProperties.addFromMap(brooklynAdditionalProperties); } - ((ManagementContextInternal)managementContext).setCatalogInitialization(catalogInitialization); + if (catalogInitialization!=null) { + ((ManagementContextInternal)managementContext).setCatalogInitialization(catalogInitialization); + } if (customizeManagement!=null) { customizeManagement.apply(managementContext); @@ -1009,7 +1011,7 @@ public class BrooklynLauncher { if (managementContext.getHighAvailabilityManager().getPersister() != null) { managementContext.getHighAvailabilityManager().getPersister().waitForWritesCompleted(Duration.TEN_SECONDS); } - managementContext.getRebindManager().waitForPendingComplete(Duration.TEN_SECONDS); + managementContext.getRebindManager().waitForPendingComplete(Duration.TEN_SECONDS, true); LOG.info("Finished waiting for persist; took "+Time.makeTimeStringRounded(stopwatch)); } catch (RuntimeInterruptedException e) { Thread.currentThread().interrupt(); // keep going with shutdown http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/d6ae0511/usage/rest-server/src/main/java/brooklyn/rest/resources/ServerResource.java ---------------------------------------------------------------------- diff --git a/usage/rest-server/src/main/java/brooklyn/rest/resources/ServerResource.java b/usage/rest-server/src/main/java/brooklyn/rest/resources/ServerResource.java index 03d51fc..abfb864 100644 --- a/usage/rest-server/src/main/java/brooklyn/rest/resources/ServerResource.java +++ b/usage/rest-server/src/main/java/brooklyn/rest/resources/ServerResource.java @@ -176,11 +176,14 @@ public class ServerResource extends AbstractBrooklynRestResource implements Serv if (!hasAppErrorsOrTimeout.get() || forceShutdownOnError) { //give the http request a chance to complete gracefully Time.sleep(delayForHttpReturn); + System.exit(0); + + } else { + // There are app errors, don't exit the process, allowing any exception to continue throwing + log.warn("Abandoning shutdown because there were errors and shutdown was not forced."); + } - - // There are app errors, don't exit the process, allowing any exception to continue throwing - log.warn("Abandoning shutdown because there were errors and shutdown was not forced."); } } http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/d6ae0511/usage/rest-server/src/test/java/brooklyn/rest/HaMasterCheckFilterTest.java ---------------------------------------------------------------------- diff --git a/usage/rest-server/src/test/java/brooklyn/rest/HaMasterCheckFilterTest.java b/usage/rest-server/src/test/java/brooklyn/rest/HaMasterCheckFilterTest.java index 27d8d6c..eabeef1 100644 --- a/usage/rest-server/src/test/java/brooklyn/rest/HaMasterCheckFilterTest.java +++ b/usage/rest-server/src/test/java/brooklyn/rest/HaMasterCheckFilterTest.java @@ -152,7 +152,7 @@ System.err.println("TEAR DOWN"); writeMgmt = createManagementContext(mementoDir, writeMode); appId = createApp(writeMgmt); - writeMgmt.getRebindManager().waitForPendingComplete(TIMEOUT); + writeMgmt.getRebindManager().waitForPendingComplete(TIMEOUT, true); if (readMode == HighAvailabilityMode.DISABLED) { //no HA, one node only http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/d6ae0511/utils/common/src/main/java/brooklyn/util/javalang/AggregateClassLoader.java ---------------------------------------------------------------------- diff --git a/utils/common/src/main/java/brooklyn/util/javalang/AggregateClassLoader.java b/utils/common/src/main/java/brooklyn/util/javalang/AggregateClassLoader.java index 1a5dd93..a3ca374 100644 --- a/utils/common/src/main/java/brooklyn/util/javalang/AggregateClassLoader.java +++ b/utils/common/src/main/java/brooklyn/util/javalang/AggregateClassLoader.java @@ -23,6 +23,7 @@ import java.net.URL; import java.util.Collection; import java.util.Collections; import java.util.Enumeration; +import java.util.Iterator; import java.util.List; import java.util.Set; import java.util.concurrent.CopyOnWriteArrayList; @@ -35,6 +36,8 @@ import com.google.common.collect.Sets; * exposing more info, a few conveniences, and a nice toString */ public class AggregateClassLoader extends ClassLoader { + // thread safe -- and all access in this class is also synchronized, + // so that reset is guaranteed not to interfere with an add(0, cl) private final CopyOnWriteArrayList<ClassLoader> classLoaders = new CopyOnWriteArrayList<ClassLoader>(); private AggregateClassLoader() { @@ -58,21 +61,38 @@ public class AggregateClassLoader extends ClassLoader { /** Add a loader to the first position in the search path. */ public void addFirst(ClassLoader classLoader) { - if (classLoader != null) classLoaders.add(0, classLoader); + if (classLoader != null) { + synchronized (classLoaders) { + classLoaders.add(0, classLoader); + } + } } /** Add a loader to the last position in the search path. */ public void addLast(ClassLoader classLoader) { - if (classLoader != null) classLoaders.add(classLoader); + if (classLoader != null) { + synchronized (classLoaders) { + classLoaders.add(classLoader); + } + } } /** Add a loader to the specific position in the search path. * (It is callers responsibility to ensure that position is valid.) */ public void add(int index, ClassLoader classLoader) { - if (classLoader != null) classLoaders.add(index, classLoader); + if (classLoader != null) { + synchronized (classLoaders) { + classLoaders.add(index, classLoader); + } + } } /** Resets the classloader shown here to be the given set */ public void reset(Collection<? extends ClassLoader> newClassLoaders) { synchronized (classLoaders) { + // synchronize: + // * to prevent concurrent invocations + // * so add(0, cl) doesn't interfere + // * and for good measure we add before removing so that iterator always contains everything + // although since iterator access is synchronized that shouldn't be necessary int count = classLoaders.size(); classLoaders.addAll(newClassLoaders); for (int i=0; i<count; i++) { @@ -93,6 +113,13 @@ public class AggregateClassLoader extends ClassLoader { return classLoaders; } + public Iterator<ClassLoader> iterator() { + synchronized (classLoaders) { + // provides iterator of snapshot + return classLoaders.iterator(); + } + } + @Override protected Class<?> findClass(String name) throws ClassNotFoundException { for (ClassLoader classLoader: classLoaders) { @@ -117,7 +144,9 @@ public class AggregateClassLoader extends ClassLoader { @Override public URL getResource(String name) { URL result = null; - for (ClassLoader classLoader: classLoaders) { + Iterator<ClassLoader> cli = iterator(); + while (cli.hasNext()) { + ClassLoader classLoader=cli.next(); result = classLoader.getResource(name); if (result!=null) return result; } @@ -131,7 +160,9 @@ public class AggregateClassLoader extends ClassLoader { @Override public Enumeration<URL> getResources(String name) throws IOException { Set<URL> resources = Sets.newLinkedHashSet(); - for (ClassLoader classLoader : classLoaders) { + Iterator<ClassLoader> cli = iterator(); + while (cli.hasNext()) { + ClassLoader classLoader=cli.next(); resources.addAll(Collections.list(classLoader.getResources(name))); } return Collections.enumeration(resources);
