You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@isis.apache.org by ah...@apache.org on 2020/01/22 13:10:39 UTC
[isis] 02/02: ISIS-2158: supposed thread-safety fix for EntityPage
serialization (2)
This is an automated email from the ASF dual-hosted git repository.
ahuber pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/isis.git
commit 5d3388d85f043616bffe630e2ee12da66e1f941b
Author: Andi Huber <ah...@apache.org>
AuthorDate: Wed Jan 22 14:10:26 2020 +0100
ISIS-2158: supposed thread-safety fix for EntityPage serialization (2)
---
.../layout/component/ActionLayoutDataOwner.java | 4 +-
.../layout/component/CollectionLayoutData.java | 22 +++----
.../isis/applib/layout/component/FieldSet.java | 31 +++++----
.../layout/component/PropertyLayoutData.java | 14 ++--
.../isis/applib/layout/grid/bootstrap3/BS3Col.java | 15 +++--
.../concurrent/_ConcurrentListWrapper.java | 77 ++++++++++++++++++++++
.../commons/internal/exceptions/_Exceptions.java | 35 ++++++++++
.../services/grid/GridLoaderServiceDefault.java | 16 +++--
.../grid/bootstrap3/GridSystemServiceBS3.java | 10 +--
9 files changed, 173 insertions(+), 51 deletions(-)
diff --git a/api/applib/src/main/java/org/apache/isis/applib/layout/component/ActionLayoutDataOwner.java b/api/applib/src/main/java/org/apache/isis/applib/layout/component/ActionLayoutDataOwner.java
index c4b07bb..c4153d0 100644
--- a/api/applib/src/main/java/org/apache/isis/applib/layout/component/ActionLayoutDataOwner.java
+++ b/api/applib/src/main/java/org/apache/isis/applib/layout/component/ActionLayoutDataOwner.java
@@ -21,6 +21,8 @@ package org.apache.isis.applib.layout.component;
import java.util.List;
public interface ActionLayoutDataOwner extends Owner {
+
List<ActionLayoutData> getActions();
- void setActions(List<ActionLayoutData> actions);
+ void addAction(ActionLayoutData actionLayoutData);
+
}
diff --git a/api/applib/src/main/java/org/apache/isis/applib/layout/component/CollectionLayoutData.java b/api/applib/src/main/java/org/apache/isis/applib/layout/component/CollectionLayoutData.java
index 90a4e34..6991c75 100644
--- a/api/applib/src/main/java/org/apache/isis/applib/layout/component/CollectionLayoutData.java
+++ b/api/applib/src/main/java/org/apache/isis/applib/layout/component/CollectionLayoutData.java
@@ -23,13 +23,14 @@ import java.util.List;
import javax.xml.bind.annotation.XmlAttribute;
import javax.xml.bind.annotation.XmlElement;
+import javax.xml.bind.annotation.XmlElementRef;
import javax.xml.bind.annotation.XmlRootElement;
import javax.xml.bind.annotation.XmlTransient;
import javax.xml.bind.annotation.XmlType;
import org.apache.isis.applib.annotation.Where;
import org.apache.isis.applib.layout.links.Link;
-import org.apache.isis.core.commons.collections.Can;
+import org.apache.isis.core.commons.internal.concurrent._ConcurrentListWrapper;
/**
* Describes the layout of a single collection, broadly corresponds to the {@link org.apache.isis.applib.annotation.CollectionLayout} annotation.
@@ -196,23 +197,22 @@ HasCssClass, HasDescribedAs, HasHidden, HasNamed {
this.sortedBy = sortedBy;
}
- private final Object $actionsLock = new Object[0]; // serializable
- private Can<ActionLayoutData> actions = Can.empty(); // serializable
+ private _ConcurrentListWrapper<ActionLayoutData> actions = new _ConcurrentListWrapper<>();
// no wrapper
@Override
- @XmlElement(name = "action", required = false)
+ @XmlElementRef(type = ActionLayoutData.class, name = "action", required = false)
public List<ActionLayoutData> getActions() {
- synchronized ($actionsLock) {
- return actions.toList();
- }
+ return actions.snapshot();
}
- @Override
public void setActions(List<ActionLayoutData> actionLayoutDatas) {
- synchronized ($actionsLock) {
- this.actions = Can.ofCollection(actionLayoutDatas);
- }
+ this.actions.replace(actionLayoutDatas);
+ }
+
+ @Override
+ public void addAction(ActionLayoutData actionLayoutData) {
+ this.actions.add(actionLayoutData);
}
private CollectionLayoutDataOwner owner;
diff --git a/api/applib/src/main/java/org/apache/isis/applib/layout/component/FieldSet.java b/api/applib/src/main/java/org/apache/isis/applib/layout/component/FieldSet.java
index 335d65a..c5e071b 100644
--- a/api/applib/src/main/java/org/apache/isis/applib/layout/component/FieldSet.java
+++ b/api/applib/src/main/java/org/apache/isis/applib/layout/component/FieldSet.java
@@ -19,7 +19,6 @@
package org.apache.isis.applib.layout.component;
import java.io.Serializable;
-import java.util.ArrayList;
import java.util.List;
import javax.xml.bind.annotation.XmlAttribute;
@@ -30,6 +29,7 @@ import javax.xml.bind.annotation.XmlType;
import org.apache.isis.applib.annotation.MemberOrder;
import org.apache.isis.applib.layout.grid.bootstrap3.BS3Col;
+import org.apache.isis.core.commons.internal.concurrent._ConcurrentListWrapper;
/**
* A {@link MemberRegion region} of the page containing a set of
@@ -133,36 +133,39 @@ Serializable {
this.name = name;
}
-
-
- private List<ActionLayoutData> actions = new ArrayList<>();
+ private _ConcurrentListWrapper<ActionLayoutData> actions = new _ConcurrentListWrapper<>();
// no wrapper
@Override
@XmlElement(name = "action", required = false)
public List<ActionLayoutData> getActions() {
- return actions;
+ return actions.snapshot();
}
- @Override
public void setActions(List<ActionLayoutData> actionLayoutDatas) {
- this.actions = actionLayoutDatas;
+ this.actions.replace(actionLayoutDatas);
}
-
-
-
- private List<PropertyLayoutData> properties = new ArrayList<>();
+
+ @Override
+ public void addAction(ActionLayoutData actionLayoutData) {
+ this.actions.add(actionLayoutData);
+ }
+
+ private _ConcurrentListWrapper<PropertyLayoutData> properties = new _ConcurrentListWrapper<>();
// no wrapper; required=false because may be auto-generated
@XmlElement(name = "property", required = false)
public List<PropertyLayoutData> getProperties() {
- return properties;
+ return properties.snapshot();
}
public void setProperties(List<PropertyLayoutData> properties) {
- this.properties = properties;
+ this.properties.replace(properties);
+ }
+
+ public void addProperty(PropertyLayoutData actionLayoutData) {
+ this.properties.add(actionLayoutData);
}
-
private FieldSetOwner owner;
/**
diff --git a/api/applib/src/main/java/org/apache/isis/applib/layout/component/PropertyLayoutData.java b/api/applib/src/main/java/org/apache/isis/applib/layout/component/PropertyLayoutData.java
index 817a6aa..a782ff4 100644
--- a/api/applib/src/main/java/org/apache/isis/applib/layout/component/PropertyLayoutData.java
+++ b/api/applib/src/main/java/org/apache/isis/applib/layout/component/PropertyLayoutData.java
@@ -19,7 +19,6 @@
package org.apache.isis.applib.layout.component;
import java.io.Serializable;
-import java.util.ArrayList;
import java.util.List;
import javax.xml.bind.annotation.XmlAttribute;
@@ -34,6 +33,7 @@ import org.apache.isis.applib.annotation.RenderDay;
import org.apache.isis.applib.annotation.Repainting;
import org.apache.isis.applib.annotation.Where;
import org.apache.isis.applib.layout.links.Link;
+import org.apache.isis.core.commons.internal.concurrent._ConcurrentListWrapper;
/**
* Describes the layout of a single property, broadly corresponds to the {@link org.apache.isis.applib.annotation.PropertyLayout} annotation.
@@ -234,18 +234,22 @@ HasCssClass, HasDescribedAs, HasHidden, HasNamed {
this.repainting = repainting;
}
- private List<ActionLayoutData> actions = new ArrayList<>();
+ private _ConcurrentListWrapper<ActionLayoutData> actions = new _ConcurrentListWrapper<>();
// no wrapper
@Override
@XmlElement(name = "action", required = false)
public List<ActionLayoutData> getActions() {
- return actions;
+ return actions.snapshot();
}
- @Override
public void setActions(List<ActionLayoutData> actionLayoutDatas) {
- this.actions = actionLayoutDatas;
+ this.actions.replace(actionLayoutDatas);
+ }
+
+ @Override
+ public void addAction(ActionLayoutData actionLayoutData) {
+ this.actions.add(actionLayoutData);
}
diff --git a/api/applib/src/main/java/org/apache/isis/applib/layout/grid/bootstrap3/BS3Col.java b/api/applib/src/main/java/org/apache/isis/applib/layout/grid/bootstrap3/BS3Col.java
index 7f44432..439b9b1 100644
--- a/api/applib/src/main/java/org/apache/isis/applib/layout/grid/bootstrap3/BS3Col.java
+++ b/api/applib/src/main/java/org/apache/isis/applib/layout/grid/bootstrap3/BS3Col.java
@@ -35,6 +35,7 @@ import org.apache.isis.applib.layout.component.DomainObjectLayoutData;
import org.apache.isis.applib.layout.component.DomainObjectLayoutDataOwner;
import org.apache.isis.applib.layout.component.FieldSet;
import org.apache.isis.applib.layout.component.FieldSetOwner;
+import org.apache.isis.core.commons.internal.concurrent._ConcurrentListWrapper;
/**
* A column within a row which, depending on its {@link #getSpan()}, could be as narrow as 1/12th of the page's width, all the way up to spanning the entire page.
@@ -171,23 +172,25 @@ CollectionLayoutDataOwner, DomainObjectLayoutDataOwner {
}
-
- private List<ActionLayoutData> actions = new ArrayList<>();
+ private _ConcurrentListWrapper<ActionLayoutData> actions = new _ConcurrentListWrapper<>();
// no wrapper
@Override
@XmlElementRef(type = ActionLayoutData.class, name = "action", required = false)
public List<ActionLayoutData> getActions() {
- return actions;
+ return actions.snapshot();
}
+ public void setActions(List<ActionLayoutData> actionLayoutDatas) {
+ this.actions.replace(actionLayoutDatas);
+ }
+
@Override
- public void setActions(final List<ActionLayoutData> actions) {
- this.actions = actions;
+ public void addAction(ActionLayoutData actionLayoutData) {
+ this.actions.add(actionLayoutData);
}
-
private List<BS3Row> rows = new ArrayList<>();
// no wrapper
diff --git a/core/commons/src/main/java/org/apache/isis/core/commons/internal/concurrent/_ConcurrentListWrapper.java b/core/commons/src/main/java/org/apache/isis/core/commons/internal/concurrent/_ConcurrentListWrapper.java
new file mode 100644
index 0000000..edc1d4e
--- /dev/null
+++ b/core/commons/src/main/java/org/apache/isis/core/commons/internal/concurrent/_ConcurrentListWrapper.java
@@ -0,0 +1,77 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.isis.core.commons.internal.concurrent;
+
+import java.io.Serializable;
+import java.util.ArrayList;
+import java.util.List;
+
+import javax.annotation.Nullable;
+
+/**
+ * <h1>- internal use only -</h1>
+ *
+ * Provides limited thread-safe access to a list object, allowing add/replace/clear/snapshot.
+ * <p>
+ * Serializable
+ *
+ * <p>
+ * <b>WARNING</b>: Do <b>NOT</b> use any of the classes provided by this package! <br/>
+ * These may be changed or removed without notice!
+ * </p>
+ *
+ * @since 2.0
+ */
+public class _ConcurrentListWrapper<T> implements Serializable {
+
+ private static final long serialVersionUID = 1L;
+
+ private final Object $lock = new Object[0]; // serializable
+ private List<T> list = new ArrayList<>(); // serializable
+
+ public List<T> snapshot() {
+ synchronized ($lock) {
+ return new ArrayList<>(list);
+ }
+ }
+
+ public void replace(@Nullable List<T> list) {
+ synchronized ($lock) {
+ this.list = list!=null
+ ? new ArrayList<>(list)
+ : new ArrayList<>();
+ }
+ }
+
+ public void clear() {
+ synchronized ($lock) {
+ this.list = new ArrayList<>(); // allow garbage collection of the old one
+ }
+ }
+
+ public void add(@Nullable T element) {
+ if(element==null) {
+ return; // noop
+ }
+ synchronized ($lock) {
+ this.list.add(element);
+ }
+ }
+
+}
diff --git a/core/commons/src/main/java/org/apache/isis/core/commons/internal/exceptions/_Exceptions.java b/core/commons/src/main/java/org/apache/isis/core/commons/internal/exceptions/_Exceptions.java
index fd1f314..eb2ba12 100644
--- a/core/commons/src/main/java/org/apache/isis/core/commons/internal/exceptions/_Exceptions.java
+++ b/core/commons/src/main/java/org/apache/isis/core/commons/internal/exceptions/_Exceptions.java
@@ -31,6 +31,7 @@ import java.util.stream.Stream;
import javax.annotation.Nullable;
+import org.apache.isis.core.commons.collections.Can;
import org.apache.isis.core.commons.exceptions.IsisException;
import org.apache.isis.core.commons.internal.base._NullSafe;
import org.apache.isis.core.commons.internal.base._Strings;
@@ -132,6 +133,38 @@ public final class _Exceptions {
return new UnsupportedOperationException("unrecoverable error: method call not allowed/supported");
}
+ // -- MESSAGE
+
+ public static String getMessage(Exception ex) {
+ if(ex==null) {
+ return "no exception present";
+ }
+ if(_Strings.isNotEmpty(ex.getMessage())) {
+ return ex.getMessage();
+ }
+ val sb = new StringBuilder();
+ val nestedMsg = streamCausalChain(ex)
+ .peek(throwable->{
+ sb.append(throwable.getClass().getSimpleName()).append("/");
+ })
+ .map(Throwable::getMessage)
+ .filter(_NullSafe::isPresent)
+ .findFirst();
+
+ if(nestedMsg.isPresent()) {
+ sb.append(nestedMsg.get());
+ } else {
+
+ Can.ofArray(ex.getStackTrace())
+ .stream()
+ .limit(20)
+ .forEach(trace->sb.append("\n").append(trace));
+ }
+
+ return sb.toString();
+ }
+
+ // -- THROWING
/**
* Used to hide from the compiler the fact, that this call always throws.
@@ -405,6 +438,8 @@ public final class _Exceptions {
}
}
+
+
}
diff --git a/core/metamodel/src/main/java/org/apache/isis/core/metamodel/services/grid/GridLoaderServiceDefault.java b/core/metamodel/src/main/java/org/apache/isis/core/metamodel/services/grid/GridLoaderServiceDefault.java
index 16c1ecb..1819b9e 100644
--- a/core/metamodel/src/main/java/org/apache/isis/core/metamodel/services/grid/GridLoaderServiceDefault.java
+++ b/core/metamodel/src/main/java/org/apache/isis/core/metamodel/services/grid/GridLoaderServiceDefault.java
@@ -39,8 +39,10 @@ import org.apache.isis.applib.services.message.MessageService;
import org.apache.isis.core.commons.internal.collections._Lists;
import org.apache.isis.core.commons.internal.collections._Maps;
import org.apache.isis.core.commons.internal.environment.IsisSystemEnvironment;
+import org.apache.isis.core.commons.internal.exceptions._Exceptions;
import org.apache.isis.core.commons.internal.resources._Resources;
+import lombok.val;
import lombok.extern.log4j.Log4j2;
@Service
@@ -51,7 +53,10 @@ import lombok.extern.log4j.Log4j2;
@Log4j2
public class GridLoaderServiceDefault implements GridLoaderService {
-
+ @Inject private MessageService messageService;
+ @Inject private GridReaderUsingJaxb gridReader;
+ @Inject private IsisSystemEnvironment isisSystemEnvironment;
+
static class DomainClassAndLayout {
private final Class<?> domainClass;
private final String layoutIfAny;
@@ -183,6 +188,8 @@ public class GridLoaderServiceDefault implements GridLoaderService {
}
return grid;
} catch(Exception ex) {
+
+ val exceptionMessage = _Exceptions.getMessage(ex);
if(supportsReloading()) {
// save fact that this was bad XML, so that we don't log again if called next time
@@ -192,7 +199,7 @@ public class GridLoaderServiceDefault implements GridLoaderService {
// note that we don't blacklist if the file exists but couldn't be parsed;
// the developer might fix so we will want to retry.
final String resourceName = resourceNameFor(dcal);
- final String message = "Failed to parse " + resourceName + " file (" + ex.getMessage() + ")";
+ final String message = "Failed to parse " + resourceName + " file (" + exceptionMessage + ")";
if(supportsReloading()) {
messageService.warnUser(message);
}
@@ -248,10 +255,5 @@ public class GridLoaderServiceDefault implements GridLoaderService {
return null;
}
- // -- DEPENDENCIES
-
- @Inject MessageService messageService;
- @Inject GridReaderUsingJaxb gridReader;
- @Inject IsisSystemEnvironment isisSystemEnvironment;
}
diff --git a/core/metamodel/src/main/java/org/apache/isis/core/metamodel/services/grid/bootstrap3/GridSystemServiceBS3.java b/core/metamodel/src/main/java/org/apache/isis/core/metamodel/services/grid/bootstrap3/GridSystemServiceBS3.java
index 0c9d0d0..0e3fafa 100644
--- a/core/metamodel/src/main/java/org/apache/isis/core/metamodel/services/grid/bootstrap3/GridSystemServiceBS3.java
+++ b/core/metamodel/src/main/java/org/apache/isis/core/metamodel/services/grid/bootstrap3/GridSystemServiceBS3.java
@@ -521,7 +521,7 @@ public class GridSystemServiceBS3 extends GridSystemServiceAbstract<BS3Grid> {
// action to the property
val collectionLayoutData = collectionLayoutDataById.get(id);
if(collectionLayoutData==null) {
- log.warn("failed to lookup CollectionLayoutData by id '{}'", id);
+ log.warn("Failed to lookup CollectionLayoutData by id '{}'", id);
} else {
val actionLayoutData = new ActionLayoutData(actionId);
addActionTo(collectionLayoutData, actionLayoutData);
@@ -582,7 +582,7 @@ public class GridSystemServiceBS3 extends GridSystemServiceAbstract<BS3Grid> {
for (final String propertyId : propertyIds) {
if(!existingIds.contains(propertyId)) {
final PropertyLayoutData propertyLayoutData = new PropertyLayoutData(propertyId);
- fieldSet.getProperties().add(propertyLayoutData);
+ fieldSet.addProperty(propertyLayoutData);
propertyLayoutData.setOwner(fieldSet);
propertyLayoutDataById.put(propertyId, propertyLayoutData);
}
@@ -652,11 +652,7 @@ public class GridSystemServiceBS3 extends GridSystemServiceAbstract<BS3Grid> {
private void addActionTo(
final ActionLayoutDataOwner owner,
final ActionLayoutData actionLayoutData) {
- List<ActionLayoutData> actions = owner.getActions();
- if(actions == null) {
- owner.setActions(actions = _Lists.newArrayList());
- }
- actions.add(actionLayoutData);
+ owner.addAction(actionLayoutData);
actionLayoutData.setOwner(owner);
}