You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@wicket.apache.org by ad...@apache.org on 2015/06/04 15:56:52 UTC
[4/9] wicket git commit: WICKET-5916 StackOverflowError in
LoadableDetachableModel
WICKET-5916 StackOverflowError in LoadableDetachableModel
This fixes a circular calling issue with LoadableDetachableModel where
getObject() is called from the context of a load(). By tracking the
state of the LoadableDetachableModel more closely, we can short-circuit
the call chain and break out of the infinite calling loop.
I've opted to use an enum instead of adding a new flag to keep the
amount of flags to a minimum and to keep the memory requirements
low (instead of 2 flags, just one enum is referenced). The external
public and protected interfaces are still the same, so I don't
expect any problems within user applications, unless they are
manipulating the attached flag using reflection.
Fixes WICKET-5916
Project: http://git-wip-us.apache.org/repos/asf/wicket/repo
Commit: http://git-wip-us.apache.org/repos/asf/wicket/commit/f7767838
Tree: http://git-wip-us.apache.org/repos/asf/wicket/tree/f7767838
Diff: http://git-wip-us.apache.org/repos/asf/wicket/diff/f7767838
Branch: refs/heads/WICKET-5906-7.x
Commit: f7767838f0c47a4947404b87b2747f2eaecf1878
Parents: 79a3d50
Author: Martijn Dashorst <ma...@gmail.com>
Authored: Fri May 29 19:11:28 2015 +0200
Committer: Andrea Del Bene <“adelbene@apache.org”>
Committed: Wed Jun 3 10:04:20 2015 +0200
----------------------------------------------------------------------
.../wicket/model/LoadableDetachableModel.java | 65 +++++++++++----
.../model/LoadableDetachableModelTest.java | 87 ++++++++++++++++++++
2 files changed, 138 insertions(+), 14 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/wicket/blob/f7767838/wicket-core/src/main/java/org/apache/wicket/model/LoadableDetachableModel.java
----------------------------------------------------------------------
diff --git a/wicket-core/src/main/java/org/apache/wicket/model/LoadableDetachableModel.java b/wicket-core/src/main/java/org/apache/wicket/model/LoadableDetachableModel.java
index c172678..e3a7fc0 100644
--- a/wicket-core/src/main/java/org/apache/wicket/model/LoadableDetachableModel.java
+++ b/wicket-core/src/main/java/org/apache/wicket/model/LoadableDetachableModel.java
@@ -23,8 +23,8 @@ import org.slf4j.LoggerFactory;
/**
* Model that makes working with detachable models a breeze. LoadableDetachableModel holds a
- * temporary, transient model object, that is set when {@link #getObject()} is called by
- * calling abstract method 'load', and that will be reset/ set to null on {@link #detach()}.
+ * temporary, transient model object, that is set when {@link #getObject()} is called by calling
+ * abstract method 'load', and that will be reset/ set to null on {@link #detach()}.
*
* A usage example:
*
@@ -60,8 +60,40 @@ public abstract class LoadableDetachableModel<T> implements IModel<T>
/** Logger. */
private static final Logger log = LoggerFactory.getLogger(LoadableDetachableModel.class);
+ private enum AttachingState
+ {
+ DETACHED(false, false),
+ ATTACHING(true, false),
+ ATTACHED(true, true);
+
+ private boolean attaching;
+ private boolean attached;
+
+ private AttachingState(boolean attaching, boolean attached)
+ {
+ this.attached = attached;
+ this.attaching = attaching;
+ }
+
+ public boolean isAttached()
+ {
+ return attached;
+ }
+
+ public boolean isAttaching()
+ {
+ return attaching;
+ }
+
+ @Override
+ public String toString()
+ {
+ return name().toLowerCase();
+ }
+ }
+
/** keeps track of whether this model is attached or detached */
- private transient boolean attached = false;
+ private transient AttachingState attached = AttachingState.DETACHED;
/** temporary, transient object. */
private transient T transientModelObject;
@@ -83,7 +115,7 @@ public abstract class LoadableDetachableModel<T> implements IModel<T>
public LoadableDetachableModel(T object)
{
this.transientModelObject = object;
- attached = true;
+ attached = AttachingState.ATTACHED;
}
/**
@@ -92,7 +124,7 @@ public abstract class LoadableDetachableModel<T> implements IModel<T>
@Override
public void detach()
{
- if (attached)
+ if (attached == AttachingState.ATTACHED)
{
try
{
@@ -100,7 +132,7 @@ public abstract class LoadableDetachableModel<T> implements IModel<T>
}
finally
{
- attached = false;
+ attached = AttachingState.DETACHED;
transientModelObject = null;
log.debug("removed transient object for {}, requestCycle {}", this,
@@ -115,8 +147,11 @@ public abstract class LoadableDetachableModel<T> implements IModel<T>
@Override
public final T getObject()
{
- if (!attached)
+ if (attached == AttachingState.DETACHED)
{
+ // prevent infinite attachment loops
+ attached = AttachingState.ATTACHING;
+
transientModelObject = load();
if (log.isDebugEnabled())
@@ -125,7 +160,7 @@ public abstract class LoadableDetachableModel<T> implements IModel<T>
", requestCycle " + RequestCycle.get());
}
- attached = true;
+ attached = AttachingState.ATTACHED;
onAttach();
}
return transientModelObject;
@@ -138,7 +173,7 @@ public abstract class LoadableDetachableModel<T> implements IModel<T>
*/
public final boolean isAttached()
{
- return attached;
+ return attached.isAttached();
}
/**
@@ -147,9 +182,12 @@ public abstract class LoadableDetachableModel<T> implements IModel<T>
@Override
public String toString()
{
- StringBuilder sb = new StringBuilder(super.toString());
- sb.append(":attached=").append(attached).append(":tempModelObject=[").append(
- this.transientModelObject).append("]");
+ StringBuilder sb = new StringBuilder(super.toString());
+ sb.append(":attached=")
+ .append(isAttached())
+ .append(":tempModelObject=[")
+ .append(this.transientModelObject)
+ .append("]");
return sb.toString();
}
@@ -187,8 +225,7 @@ public abstract class LoadableDetachableModel<T> implements IModel<T>
@Override
public void setObject(final T object)
{
- attached = true;
+ attached = AttachingState.ATTACHED;
transientModelObject = object;
}
-
}
http://git-wip-us.apache.org/repos/asf/wicket/blob/f7767838/wicket-core/src/test/java/org/apache/wicket/model/LoadableDetachableModelTest.java
----------------------------------------------------------------------
diff --git a/wicket-core/src/test/java/org/apache/wicket/model/LoadableDetachableModelTest.java b/wicket-core/src/test/java/org/apache/wicket/model/LoadableDetachableModelTest.java
new file mode 100644
index 0000000..a087a78
--- /dev/null
+++ b/wicket-core/src/test/java/org/apache/wicket/model/LoadableDetachableModelTest.java
@@ -0,0 +1,87 @@
+/*
+ * 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.wicket.model;
+
+import static org.hamcrest.core.Is.is;
+
+import org.apache.wicket.WicketTestCase;
+import org.junit.Test;
+
+/**
+ * Tests the states of a LoadableDetachableModel
+ */
+public class LoadableDetachableModelTest extends WicketTestCase
+{
+ /**
+ * Checks whether the LDM can escape recursive calls.
+ */
+ @Test
+ public void recursiveGetObjectDoesntCauseInfiteLoop()
+ {
+ class RecursiveLoad extends LoadableDetachableModel<Integer>
+ {
+ private static final long serialVersionUID = 1L;
+
+ private int count = 0;
+
+ @Override
+ protected Integer load()
+ {
+ count++;
+ getObject();
+ return count;
+ }
+ }
+
+ RecursiveLoad ldm = new RecursiveLoad();
+
+ assertThat(ldm.isAttached(), is(false));
+ assertThat(ldm.getObject(), is(1));
+ assertThat(ldm.isAttached(), is(true));
+ }
+
+ /**
+ * Checks whether the LDM can escape recursive calls.
+ */
+ @Test
+ public void exceptionDuringLoadKeepsLDMDetached()
+ {
+ class ExceptionalLoad extends LoadableDetachableModel<Integer>
+ {
+ private static final long serialVersionUID = 1L;
+
+ @Override
+ protected Integer load()
+ {
+ throw new RuntimeException();
+ }
+ }
+
+ ExceptionalLoad ldm = new ExceptionalLoad();
+
+ assertThat(ldm.isAttached(), is(false));
+ try
+ {
+ assertThat(ldm.getObject(), is(1));
+ fail("shouldn't get here");
+ }
+ catch (RuntimeException e)
+ {
+ assertThat(ldm.isAttached(), is(false));
+ }
+ }
+}