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/5049aa85 Tree: http://git-wip-us.apache.org/repos/asf/wicket/tree/5049aa85 Diff: http://git-wip-us.apache.org/repos/asf/wicket/diff/5049aa85 Branch: refs/heads/WICKET-5906-7.x Commit: 5049aa8525e6ee268c3e78ec77dc7012da13e51c Parents: 55cfd27 Author: Martijn Dashorst <martijn.dasho...@gmail.com> Authored: Fri May 29 19:11:28 2015 +0200 Committer: Andrea Del Bene <âadelb...@apache.orgâ> Committed: Thu Jun 4 16:19:29 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/5049aa85/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/5049aa85/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)); + } + } +}
