You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@brooklyn.apache.org by sv...@apache.org on 2017/06/08 09:37:44 UTC
[10/11] brooklyn-server git commit: tidied up names and better
comments as per PR review
tidied up names and better comments as per PR review
Project: http://git-wip-us.apache.org/repos/asf/brooklyn-server/repo
Commit: http://git-wip-us.apache.org/repos/asf/brooklyn-server/commit/3a2bf5ac
Tree: http://git-wip-us.apache.org/repos/asf/brooklyn-server/tree/3a2bf5ac
Diff: http://git-wip-us.apache.org/repos/asf/brooklyn-server/diff/3a2bf5ac
Branch: refs/heads/master
Commit: 3a2bf5aca514feacb9800ab74d0926609941a57f
Parents: 9d448f3
Author: Alex Heneveld <al...@cloudsoftcorp.com>
Authored: Thu Jun 8 00:30:04 2017 +0100
Committer: Alex Heneveld <al...@cloudsoftcorp.com>
Committed: Thu Jun 8 00:30:04 2017 +0100
----------------------------------------------------------------------
...rFromStackOfBrooklynClassLoadingContext.java | 14 ++++++--
.../core/mgmt/persist/XmlMementoSerializer.java | 1 +
.../util/core/xstream/ClassRenamingMapper.java | 7 +---
.../util/core/xstream/OsgiClassnameMapper.java | 2 ++
.../util/core/xstream/XmlSerializer.java | 36 +++++++++++++-------
5 files changed, 39 insertions(+), 21 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/3a2bf5ac/core/src/main/java/org/apache/brooklyn/core/mgmt/classloading/ClassLoaderFromStackOfBrooklynClassLoadingContext.java
----------------------------------------------------------------------
diff --git a/core/src/main/java/org/apache/brooklyn/core/mgmt/classloading/ClassLoaderFromStackOfBrooklynClassLoadingContext.java b/core/src/main/java/org/apache/brooklyn/core/mgmt/classloading/ClassLoaderFromStackOfBrooklynClassLoadingContext.java
index 102a27c..b4a3675 100644
--- a/core/src/main/java/org/apache/brooklyn/core/mgmt/classloading/ClassLoaderFromStackOfBrooklynClassLoadingContext.java
+++ b/core/src/main/java/org/apache/brooklyn/core/mgmt/classloading/ClassLoaderFromStackOfBrooklynClassLoadingContext.java
@@ -68,7 +68,15 @@ public class ClassLoaderFromStackOfBrooklynClassLoadingContext extends ClassLoad
return findClass(name);
}
- /** Must be accompanied by a corresponding {@link #popClassLoadingContext()} when finished. */
+ /** Must be accompanied by a corresponding {@link #popClassLoadingContext()} when finished.
+ * <p>
+ * This class enforces a requirement that the pop be done by the same thread that pushed.
+ * This is to help ensure a consumer does not accidentally have two threads
+ * pushing and popping for different purposes as that would completely break the stack.
+ * We were uncertain whether xstream might parallelise things so wanted this to throw
+ * if a cross-thread pop happened; there is no deeper reason. If there is a compelling
+ * use case for a single logical push-pop chain to be done by different threads we could drop this requirement.
+ */
@SuppressWarnings("deprecation")
public void pushClassLoadingContext(BrooklynClassLoadingContext clcNew) {
acquireLock();
@@ -88,7 +96,7 @@ public class ClassLoaderFromStackOfBrooklynClassLoadingContext extends ClassLoad
public void popClassLoadingContext() {
synchronized (lockOwner) {
- releaseXstreamLock();
+ releaseLock();
setCurrentClassLoader(cls.pop());
contexts.pop();
}
@@ -120,7 +128,7 @@ public class ClassLoaderFromStackOfBrooklynClassLoadingContext extends ClassLoad
}
}
- protected void releaseXstreamLock() {
+ protected void releaseLock() {
synchronized (lockOwner) {
if (lockCount<=0) {
throw new IllegalStateException("not locked");
http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/3a2bf5ac/core/src/main/java/org/apache/brooklyn/core/mgmt/persist/XmlMementoSerializer.java
----------------------------------------------------------------------
diff --git a/core/src/main/java/org/apache/brooklyn/core/mgmt/persist/XmlMementoSerializer.java b/core/src/main/java/org/apache/brooklyn/core/mgmt/persist/XmlMementoSerializer.java
index 9265a1f..90679a0 100644
--- a/core/src/main/java/org/apache/brooklyn/core/mgmt/persist/XmlMementoSerializer.java
+++ b/core/src/main/java/org/apache/brooklyn/core/mgmt/persist/XmlMementoSerializer.java
@@ -137,6 +137,7 @@ public class XmlMementoSerializer<T> extends XmlSerializer<T> implements Memento
// Warning: this is called in the super-class constructor, so before this constructor -
// most fields will not be set, including "xstream" (use a supplier if you need to)
+ // See comment on superclass method.
@Override
protected MapperWrapper wrapMapperForNormalUsage(Mapper next) {
MapperWrapper mapper = super.wrapMapperForNormalUsage(next);
http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/3a2bf5ac/core/src/main/java/org/apache/brooklyn/util/core/xstream/ClassRenamingMapper.java
----------------------------------------------------------------------
diff --git a/core/src/main/java/org/apache/brooklyn/util/core/xstream/ClassRenamingMapper.java b/core/src/main/java/org/apache/brooklyn/util/core/xstream/ClassRenamingMapper.java
index 5691198..4b3a4c0 100644
--- a/core/src/main/java/org/apache/brooklyn/util/core/xstream/ClassRenamingMapper.java
+++ b/core/src/main/java/org/apache/brooklyn/util/core/xstream/ClassRenamingMapper.java
@@ -90,12 +90,7 @@ public class ClassRenamingMapper extends MapperWrapper {
public ClassRenamingMapper(Mapper wrapped, Map<String, String> nameToType, Supplier<? extends ClassLoader> classLoaderSupplier) {
super(wrapped);
this.nameToType = checkNotNull(nameToType, "nameToType");
- /*
- * NB: wanted to pass xstream, rather than Supplier<ClassLoader>, in constructor. However,
- * this caused NPE because of how this is constructed from inside
- * XmlMementoSerializer.wrapMapperForNormalUsage, called from within an anonymous subclass of XStream!
- * (Similar as for OsgiClassnameMapper.)
- */
+ // supplier, rather than instance, used for reasons noted at XmlSerializer.wrapMapperForNormalUsage
this.classLoaderSupplier = checkNotNull(classLoaderSupplier, "classLoaderSupplier");
}
http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/3a2bf5ac/core/src/main/java/org/apache/brooklyn/util/core/xstream/OsgiClassnameMapper.java
----------------------------------------------------------------------
diff --git a/core/src/main/java/org/apache/brooklyn/util/core/xstream/OsgiClassnameMapper.java b/core/src/main/java/org/apache/brooklyn/util/core/xstream/OsgiClassnameMapper.java
index f82e743..8299135 100644
--- a/core/src/main/java/org/apache/brooklyn/util/core/xstream/OsgiClassnameMapper.java
+++ b/core/src/main/java/org/apache/brooklyn/util/core/xstream/OsgiClassnameMapper.java
@@ -44,7 +44,9 @@ public class OsgiClassnameMapper extends MapperWrapper {
public OsgiClassnameMapper(Supplier<XStream> xstream, MapperWrapper mapper) {
super(mapper);
+ // supplier, rather than instance, used for reasons noted at XmlSerializer.wrapMapperForNormalUsage
this.xstream = xstream;
+
prefixer = new OsgiClassPrefixer();
}
http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/3a2bf5ac/core/src/main/java/org/apache/brooklyn/util/core/xstream/XmlSerializer.java
----------------------------------------------------------------------
diff --git a/core/src/main/java/org/apache/brooklyn/util/core/xstream/XmlSerializer.java b/core/src/main/java/org/apache/brooklyn/util/core/xstream/XmlSerializer.java
index c0349b6..073f5ad 100644
--- a/core/src/main/java/org/apache/brooklyn/util/core/xstream/XmlSerializer.java
+++ b/core/src/main/java/org/apache/brooklyn/util/core/xstream/XmlSerializer.java
@@ -101,7 +101,7 @@ public class XmlSerializer<T> {
* It is configured in XStream default *without* access to the XStream mapper,
* which is meant to apply when serializing the type name for instances of that type.
* <p>
- * However we need a few selected mappers (see {@link #wrapMapperForAllLowLevelMentions(Mapper)} )
+ * However we need a few selected mappers (see {@link #wrapMapperForHandlingClasses(Mapper)} )
* to apply to all class renames, but many of the mappers must NOT be used,
* e.g. because some might intercept all Class<? extends Entity> references
* (and that interception is only wanted when serializing <i>instances</i>,
@@ -111,18 +111,21 @@ public class XmlSerializer<T> {
* after the instance added by XStream.setupConverters()
*/
private JavaClassConverter newCustomJavaClassConverter() {
- return new JavaClassConverter(wrapMapperForAllLowLevelMentions(new DefaultMapper(xstream.getClassLoaderReference()))) {};
+ return new JavaClassConverter(wrapMapperForHandlingClasses(new DefaultMapper(xstream.getClassLoaderReference()))) {};
}
- /** Adds mappers needed for *any* reference to a class, both "normal" usage (when xstream wants a mapper)
- * and Class conversion (when xstream needs to serialize an instance of Class and doesn't have an alias).
+ /** Extension point where sub-classes can add mappers needed for handling class names.
+ * This is used by {@link #wrapMapperForNormalUsage(Mapper)} and also to set up the {@link JavaClassConverter}
+ * (see {@link #newCustomJavaClassConverter()} for what that does).
* <p>
* This should apply when nice names are used for inner classes, or classes are renamed;
- * however mappers which affect aliases or intercept references to entities are usually
- * NOT be invoked in this low-level pathway. See {@link #newCustomJavaClassConverter()}. */
- // Developer note - this is called by the xstream subclass constructor in the constructor of this class,
- // so very few fields are populated
- protected MapperWrapper wrapMapperForAllLowLevelMentions(Mapper next) {
+ * however mappers which affect field aliases or intercept references to entities are not
+ * wanted in the {@link JavaClassConverter} and so should be added by {@link #wrapMapperForNormalUsage(Mapper)}
+ * instead of this.
+ * <p>
+ * Developers note this is called from the constructor; be careful when overriding and
+ * see comment on {@link #wrapMapperForNormalUsage(Mapper)} about field availability. */
+ protected MapperWrapper wrapMapperForHandlingClasses(Mapper next) {
MapperWrapper result = new CompilerIndependentOuterClassFieldMapper(next);
Supplier<ClassLoader> classLoaderSupplier = new Supplier<ClassLoader>() {
@@ -140,10 +143,19 @@ public class XmlSerializer<T> {
return result;
}
- /** Extension point where sub-classes can add mappers wanted when instances of a class are serialized,
- * including {@link #wrapMapperForAllLowLevelMentions(Mapper)}, plus any usual domain mappings. */
+ /** Extension point where sub-classes can add mappers to set up the main {@link Mapper} given to XStream.
+ * This includes all of {@link #wrapMapperForHandlingClasses(Mapper)} plus anything wanted for normal usage.
+ * <p>
+ * Typically any non-class-name mappers wanted should be added in a subclass by overriding this field,
+ * calling this superclass method, then wrapping the result.
+ * <p>
+ * Developers note this is called from the constructor; be careful when overriding
+ * because most fields won't be available. In particular in a subclass,
+ * this method in the subclass will be invoked very early in its constructor.
+ * Fields like {@link #xstream} (and <i>anything</i> set in the subclass) won't
+ * yet be available. For this reason some mappers will need to be given a {@link Supplier} for late resolution. */
protected MapperWrapper wrapMapperForNormalUsage(Mapper next) {
- return wrapMapperForAllLowLevelMentions(next);
+ return wrapMapperForHandlingClasses(next);
}
public void serialize(Object object, Writer writer) {