Apply RequestCycle#onAfterDetach() patch to wicket-1.3.x

[Martijn Dashorst]

The following patch was crafted to enable checks for detach logic
*after* the request cycle has detached all request targets that were
part of the request. This is useful for those checks we perform in our
applications, and that I've presented in London and at the Wicket
meetup 2009. [1]

The problem is that when RequestCycle#onEndRequest is called, the
session lock has been removed, causing rather frequent race conditions
in our (expensive) checks. The proposed patch adds an additional hook
after the request targets have been detached, and further detaching of
the request ensues. See also WICKET-2020 [2]

Since 1.3.x is no longer officially supported, I'd like to propose
that I add this to 1.3.x and release 1.3.8 at a given moment (when we
incorporate additional bug fixes into the 1.3.x stream).

I am aware that we don't backport everything anymore, but I do see
that at least 1.3.8 will be released (we can't fix every bug, but
there are of course things that should be fixed).

Any objections to including this patch in 1.3.x?

Any objections to including this patch in 1.4.x?

Martijn

[1] http://www.slideshare.net/dashorst/keep-your-wicket-application-in-production
[2] http://issues.apache.org/jira/browse/WICKET-2020

Index: src/main/java/org/apache/wicket/RequestCycle.java
===================================================================
--- src/main/java/org/apache/wicket/RequestCycle.java (revision 803876)
+++ src/main/java/org/apache/wicket/RequestCycle.java (working copy)
@@ -1124,6 +1124,15 @@
  }
  }

+ try
+ {
+ onAfterDetach();
+ }
+ catch (Throwable re)
+ {
+ log.error("there was an error processing onAfterDetach", re);
+ }
+
  if (automaticallyClearFeedbackMessages)
  {
  // remove any rendered and otherwise obsolete feedback messages from
@@ -1520,6 +1529,13 @@
  }

  /**
+ * Called when the request cycle object has detached all request targets.
+ */
+ protected void onAfterDetach()
+ {
+ }
+
+ /**
  * Called when the request cycle object has finished its response
  */
  protected void onEndRequest()

[igor.vaynberg]

+1 if you call the method onAfterTargetsDetached()

is there a reason why this is needed in addition to IDetachListener for 1.4?

-igor

On Thu, Aug 13, 2009 at 6:46 AM, Martijn
Dashorst<martijn.dashorst@...> wrote:

> The following patch was crafted to enable checks for detach logic
> *after* the request cycle has detached all request targets that were
> part of the request. This is useful for those checks we perform in our
> applications, and that I've presented in London and at the Wicket
> meetup 2009. [1]
>
> The problem is that when RequestCycle#onEndRequest is called, the
> session lock has been removed, causing rather frequent race conditions
> in our (expensive) checks. The proposed patch adds an additional hook
> after the request targets have been detached, and further detaching of
> the request ensues. See also WICKET-2020 [2]
>
> Since 1.3.x is no longer officially supported, I'd like to propose
> that I add this to 1.3.x and release 1.3.8 at a given moment (when we
> incorporate additional bug fixes into the 1.3.x stream).
>
> I am aware that we don't backport everything anymore, but I do see
> that at least 1.3.8 will be released (we can't fix every bug, but
> there are of course things that should be fixed).
>
> Any objections to including this patch in 1.3.x?
>
> Any objections to including this patch in 1.4.x?
>
> Martijn
>
> [1] http://www.slideshare.net/dashorst/keep-your-wicket-application-in-production
> [2] http://issues.apache.org/jira/browse/WICKET-2020
>
> Index: src/main/java/org/apache/wicket/RequestCycle.java
> ===================================================================
> --- src/main/java/org/apache/wicket/RequestCycle.java   (revision 803876)
> +++ src/main/java/org/apache/wicket/RequestCycle.java   (working copy)
> @@ -1124,6 +1124,15 @@
>                        }
>                }
>
> +               try
> +               {
> +                       onAfterDetach();
> +               }
> +               catch (Throwable re)
> +               {
> +                       log.error("there was an error processing onAfterDetach", re);
> +               }
> +
>                if (automaticallyClearFeedbackMessages)
>                {
>                        // remove any rendered and otherwise obsolete feedback messages from
> @@ -1520,6 +1529,13 @@
>        }
>
>        /**
> +        * Called when the request cycle object has detached all request targets.
> +        */
> +       protected void onAfterDetach()
> +       {
> +       }
> +
> +       /**
>         * Called when the request cycle object has finished its response
>         */
>        protected void onEndRequest()
>

[Martijn Dashorst]

On Thu, Aug 13, 2009 at 5:05 PM, Igor Vaynberg<igor.vaynberg@...> wrote:
> +1 if you call the method onAfterTargetsDetached()
>
> is there a reason why this is needed in addition to IDetachListener for 1.4?

Yes, because IDetachListener is for components, while we want to check
if everything was detached properly, *and* no hibernate entities are
serialized with the pages. This needs to be done after everything has
been detached (request *and* response page).

We use the page serializer for that (only in dev mode)

Martijn

[Martijn Dashorst]

Updated patch with method rename:

Index: src/main/java/org/apache/wicket/RequestCycle.java
===================================================================
--- src/main/java/org/apache/wicket/RequestCycle.java (revision 803876)
+++ src/main/java/org/apache/wicket/RequestCycle.java (working copy)
@@ -1124,6 +1124,15 @@
  }
  }

+ try
+ {
+ onAfterTargetsDetached();
+ }
+ catch (Throwable re)
+ {
+ log.error("there was an error processing onAfterTargetsDetached", re);
+ }
+
  if (automaticallyClearFeedbackMessages)
  {
  // remove any rendered and otherwise obsolete feedback messages from
@@ -1520,6 +1529,13 @@
  }

  /**
+ * Called when the request cycle object has detached all request targets.
+ */
+ protected void onAfterTargetsDetached()
+ {
+ }
+
+ /**
  * Called when the request cycle object has finished its response
  */
  protected void onEndRequest()

[igor.vaynberg]

+1

-igor

On Thu, Aug 13, 2009 at 8:19 AM, Martijn
Dashorst<martijn.dashorst@...> wrote:

> Updated patch with method rename:
>
> Index: src/main/java/org/apache/wicket/RequestCycle.java
> ===================================================================
> --- src/main/java/org/apache/wicket/RequestCycle.java   (revision 803876)
> +++ src/main/java/org/apache/wicket/RequestCycle.java   (working copy)
> @@ -1124,6 +1124,15 @@
>                        }
>                }
>
> +               try
> +               {
> +                       onAfterTargetsDetached();
> +               }
> +               catch (Throwable re)
> +               {
> +                       log.error("there was an error processing onAfterTargetsDetached", re);
> +               }
> +
>                if (automaticallyClearFeedbackMessages)
>                {
>                        // remove any rendered and otherwise obsolete feedback messages from
> @@ -1520,6 +1529,13 @@
>        }
>
>        /**
> +        * Called when the request cycle object has detached all request targets.
> +        */
> +       protected void onAfterTargetsDetached()
> +       {
> +       }
> +
> +       /**
>         * Called when the request cycle object has finished its response
>         */
>        protected void onEndRequest()
>

[Martijn Dashorst]

I've committed the patch (I don't like having uncommitted code lying
around) to wicket-1.3.x and wicket-1.x. You may still object though,
I'll just revert the commits...

Martijn

On Thu, Aug 13, 2009 at 5:23 PM, Igor Vaynberg<igor.vaynberg@...> wrote:

> +1
>
> -igor
>
> On Thu, Aug 13, 2009 at 8:19 AM, Martijn
> Dashorst<martijn.dashorst@...> wrote:
>> Updated patch with method rename:
>>
>> Index: src/main/java/org/apache/wicket/RequestCycle.java
>> ===================================================================
>> --- src/main/java/org/apache/wicket/RequestCycle.java   (revision 803876)
>> +++ src/main/java/org/apache/wicket/RequestCycle.java   (working copy)
>> @@ -1124,6 +1124,15 @@
>>                        }
>>                }
>>
>> +               try
>> +               {
>> +                       onAfterTargetsDetached();
>> +               }
>> +               catch (Throwable re)
>> +               {
>> +                       log.error("there was an error processing onAfterTargetsDetached", re);
>> +               }
>> +
>>                if (automaticallyClearFeedbackMessages)
>>                {
>>                        // remove any rendered and otherwise obsolete feedback messages from
>> @@ -1520,6 +1529,13 @@
>>        }
>>
>>        /**
>> +        * Called when the request cycle object has detached all request targets.
>> +        */
>> +       protected void onAfterTargetsDetached()
>> +       {
>> +       }
>> +
>> +       /**
>>         * Called when the request cycle object has finished its response
>>         */
>>        protected void onEndRequest()
>>
>

--
Become a Wicket expert, learn from the best: http://wicketinaction.com
Apache Wicket 1.4 increases type safety for web applications
Get it now: http://www.apache.org/dyn/closer.cgi/wicket/1.4.0