Hi Sven,
On Wed, Aug 14, 2019 at 12:13 PM <svenme...@apache.org> wrote: > This is an automated email from the ASF dual-hosted git repository. > > svenmeier pushed a commit to branch WICKET-6558-prevent-lock-after-detach > in repository https://gitbox.apache.org/repos/asf/wicket.git > > commit 7eb27f6cb47a1a69dc4eec8ed0ce4c9039a09318 > Author: Sven Meier <svenme...@apache.org> > AuthorDate: Wed Aug 14 11:13:19 2019 +0200 > > WICKET-6558 no lock after detach > --- > .../main/java/org/apache/wicket/Application.java | 19 ++++------------- > .../src/main/java/org/apache/wicket/Session.java | 13 ++++++++++++ > .../wicket/util/tester/BaseWicketTester.java | 24 > ++++++++++++++-------- > 3 files changed, 33 insertions(+), 23 deletions(-) > > diff --git a/wicket-core/src/main/java/org/apache/wicket/Application.java > b/wicket-core/src/main/java/org/apache/wicket/Application.java > index dde8c38..d9b726a 100644 > --- a/wicket-core/src/main/java/org/apache/wicket/Application.java > +++ b/wicket-core/src/main/java/org/apache/wicket/Application.java > @@ -1569,23 +1569,12 @@ public abstract class Application implements > UnboundListener, IEventSink, IMetad > @Override > public void onDetach(final RequestCycle > requestCycle) > { > - IPageManager pageManager; > - > - if (Session.exists()) > - { > - pageManager = > Session.get().getPageManager(); > Why you dropped this code ? It is the adapted (pageAccessSynchronizer.get().adapt(manager)) page manager that calls #unlockAllPages() in its #detach() method. > - } else { > - pageManager = > internalGetPageManager(); > - } > - pageManager.detach(); > + internalGetPageManager().detach(); > > - if (Application.exists()) > + IRequestLogger requestLogger = > getRequestLogger(); > + if (requestLogger != null) > { > - IRequestLogger requestLogger = > Application.get().getRequestLogger(); > - if (requestLogger != null) > - { > - > requestLogger.requestTime((System.currentTimeMillis() - > requestCycle.getStartTime())); > - } > + > requestLogger.requestTime((System.currentTimeMillis() - > requestCycle.getStartTime())); > } } > }); > diff --git a/wicket-core/src/main/java/org/apache/wicket/Session.java > b/wicket-core/src/main/java/org/apache/wicket/Session.java > index ac37e44..3080b2b 100644 > --- a/wicket-core/src/main/java/org/apache/wicket/Session.java > +++ b/wicket-core/src/main/java/org/apache/wicket/Session.java > @@ -113,6 +113,12 @@ public abstract class Session implements > IClusterable, IEventSink, IMetadataCont > /** Logging object */ > private static final Logger log = > LoggerFactory.getLogger(Session.class); > > + /** records if pages have been unlocked for the current request */ > + private static final MetaDataKey<Boolean> PAGES_UNLOCKED = new > MetaDataKey<>() > + { > + private static final long serialVersionUID = 1L; > + }; > This metadata should be reset to false at some point, e.g. onDetach(), because on the next request it will still say "unlocked". > + > /** records if session has been invalidated by the current request > */ > private static final MetaDataKey<Boolean> SESSION_INVALIDATED = > new MetaDataKey<>() > { > @@ -670,6 +676,9 @@ public abstract class Session implements IClusterable, > IEventSink, IMetadataCont > { > detachFeedback(); > > + pageAccessSynchronizer.get().unlockAllPages(); > + RequestCycle.get().setMetaData(PAGES_UNLOCKED, true); > + > if (isSessionInvalidated()) > { > invalidateNow(); > @@ -915,6 +924,10 @@ public abstract class Session implements > IClusterable, IEventSink, IMetadataCont > */ > public final IPageManager getPageManager() > { > + if > (Boolean.TRUE.equals(RequestCycle.get().getMetaData(PAGES_UNLOCKED))) { > + throw new WicketRuntimeException("pages have > already been unlocked - synchronized access is no longer possible"); > + } > + > IPageManager manager = > Application.get().internalGetPageManager(); > return pageAccessSynchronizer.get().adapt(manager); > } > diff --git > a/wicket-core/src/main/java/org/apache/wicket/util/tester/BaseWicketTester.java > b/wicket-core/src/main/java/org/apache/wicket/util/tester/BaseWicketTester.java > index 3cf02f9..c643baa 100644 > --- > a/wicket-core/src/main/java/org/apache/wicket/util/tester/BaseWicketTester.java > +++ > b/wicket-core/src/main/java/org/apache/wicket/util/tester/BaseWicketTester.java > @@ -25,6 +25,7 @@ import java.lang.reflect.Constructor; > import java.lang.reflect.Method; > import java.nio.charset.Charset; > import java.text.ParseException; > +import java.time.Duration; > import java.util.ArrayList; > import java.util.Enumeration; > import java.util.HashMap; > @@ -123,14 +124,12 @@ import > org.apache.wicket.request.mapper.IRequestMapperDelegate; > import org.apache.wicket.request.mapper.parameter.PageParameters; > import org.apache.wicket.request.resource.IResource; > import org.apache.wicket.request.resource.ResourceReference; > -import org.apache.wicket.settings.ApplicationSettings; > import org.apache.wicket.settings.RequestCycleSettings.RenderStrategy; > import org.apache.wicket.util.lang.Args; > import org.apache.wicket.util.lang.Classes; > import org.apache.wicket.util.lang.Generics; > import org.apache.wicket.util.resource.StringResourceStream; > import org.apache.wicket.util.string.Strings; > -import java.time.Duration; > import org.apache.wicket.util.visit.IVisit; > import org.apache.wicket.util.visit.IVisitor; > import org.slf4j.Logger; > @@ -495,12 +494,21 @@ public class BaseWicketTester > */ > protected void cleanupFeedbackMessages(IFeedbackMessageFilter > filter) > { > - ApplicationSettings applicationSettings = > application.getApplicationSettings(); > - IFeedbackMessageFilter old = > applicationSettings.getFeedbackMessageCleanupFilter(); > - > applicationSettings.setFeedbackMessageCleanupFilter(filter); > - getLastRenderedPage().detach(); > - getSession().detach(); > - applicationSettings.setFeedbackMessageCleanupFilter(old); > Why these changes were needed ? Did some tests fail ? > + > + IVisitor<Component, Void> clearer = new > IVisitor<Component, Void>() > + { > + @Override > + public void component(Component component, > IVisit<Void> visit) > + { > + if (component.hasFeedbackMessage()) { > + > component.getFeedbackMessages().clear(filter); > + } > + } > + }; > + clearer.component(getLastRenderedPage(), null); > + getLastRenderedPage().visitChildren(clearer); > + > + getSession().getFeedbackMessages().clear(filter); > } > > /** > >
