On 14.08.19 14:40, Martin Grigorov wrote:
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.
The session is detached separately already and will clear its page
synchronization anyway.
It is sufficient (and clearer) when this code takes care of the
application's page manager only.
- } 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".
The metaData is set on the RequestCycle, there's no need to reset that.
+
/** 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 ?.
Yes, tests fail that want the feedback messages to be cleared:
Any later access to the session's page manager (which happens quite a
lot as you know) is invalid then, because the session was detached already.
+
+ 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);
}
/**
Thanks for you feedback. I'll create a formal pull-request later on.
Have fun
Sven
