You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@wicket.apache.org by Martin Grigorov <mg...@apache.org> on 2019/08/14 12:40:00 UTC
Re: [wicket] 01/01: WICKET-6558 no lock after detach
Hi Sven,
On Wed, Aug 14, 2019 at 12:13 PM <sv...@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 <sv...@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);
> }
>
> /**
>
>
Re: [wicket] 01/01: WICKET-6558 no lock after detach
Posted by Sven Meier <sv...@meiers.net>.
On 14.08.19 14:40, Martin Grigorov wrote:
> Hi Sven,
>
>
> On Wed, Aug 14, 2019 at 12:13 PM <sv...@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 <sv...@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