I now see it was wrong to move the detachement code to the finally block of the request cycle. With things like form submits etc. actually two requests are handled: 1. the 'preparing'/ handling of the POST, 2. the rendering, which is a GET comming from a redirect. With my code move, I had the idea of having the code on a more visible place, that is guaranteed to be executed within a request (which is not the case when it's in the component render method). On second thought, as we only want the detachment happening at rendering anyway, it was better placed in the that method.
So, I propose to move the detachement code back to component. Render will look like this then:
/**
* Performs a render of this component.
* @param cycle The response to render to
*/
public final void render(final RequestCycle cycle)
{
try
{
if(this instanceof Page)
{
initUIMessages();
} // Call implementation to render component
handleRender(visible ? cycle : cycle.nullResponse()); // Increase render count for component
rendering++;// If we're a page and the application wants component uses checked and the response is not a redirect
if (this instanceof Page && getApplicationSettings().getComponentUseCheck()
&& !cycle.getResponse().isRedirect())
{
// Visit components on page
checkRendering((Page)this);
}
}
finally // be sure to have models detached
{
// If we're a page
if (this instanceof Page)
{
// Visit components on page
final Page page = (Page)this;
detachModels(page);
}
}
}
with:
/**
* Looks if any messages were set as a temporary variable on the page
* and, if so, sets these messages as the current.
*/
private void initUIMessages()
{
// see if the page has messages attached to it
final Page page = (Page)this;
if(page.messages != null) // so, we are comming from a redirect;
// these are the saved messages from the thread that issued the redirect
{
UIMessages.set(page.messages); // set as the current threads' messages
// reset the page varaible
page.messages = null;
}
}
And the finally block of RequestCycle.render:
finally
{
release();
response.close(); //close the response
}Where release is:
/**
* Releases the current thread local related resources.
* The threadlocal of this request cycle is reset.
* If we are in a 'redirect' state, we do not want to loose our messages
* as - e.g. when handling a form - there's a fat change were comming back
*/
private void release()
{
if(getRedirect())
{
// Since we are explicitly redirecting to a page already, we do not
// want a second redirect to occur automatically
setRedirect(false);
if(page != null)
{
page.messages = UIMessages.get();
UIMessages.remove(); // only clean thread local; in fact we moved the
// reference from the thread local to the page temporarily
}
else // hmmm, no page, which probably means we are rendering directely
// ourselves; as a fallthrough, we have to clear things up
{
UIMessages.release();
}
}
else
// only detach/ release when this is not a redirect; if the redirect == true,
// it means that the handling of the total request is not done yet
{
// detach all model objects from the page
// clear the ui messages and reset the original component models
UIMessages.release();
}
current.set(null); // reset ThreadLocal reference
}
Agreed? Btw I tested it with the display tag export example, and that seems to work fine.
Eelco
Eelco Hillenius wrote:
That patch it tricky, as it does not guarantee that the models are detached. Also, the messages should be released totally, though it's not really a problem when this does not happen. Not detaching the models is very dangerous though.
Regards,
Eelco
p.s. you should have CVS access allready. Just log in using ssh (extssh in Eclipse) with you SF account.
Gwyn Evans wrote:
Should I set up CVS access, or does anyone want to review patches?
Here's this one in the meantime...
Index: src/java/com/voicetribe/wicket/RequestCycle.java
===================================================================
RCS file: /cvsroot/wicket/Wicket/src/java/com/voicetribe/wicket/RequestCycle.java,v
retrieving revision 1.10
diff -u -r1.10 RequestCycle.java
--- src/java/com/voicetribe/wicket/RequestCycle.java 29 Nov 2004
23:58:31 -0000 1.10
+++ src/java/com/voicetribe/wicket/RequestCycle.java 1 Dec 2004 11:00:43 -0000
@@ -357,8 +357,10 @@
// it means that the handling of the total request is
not done yet
{
// detach all model objects from the page
- detachModels(page);
- // clear the ui messages and reset the original component models
+ if (page != null) {
+ detachModels(page);
+ }
+ // clear the ui messages and reset the original
component models
UIMessages.release();
}
current.set(null); // reset ThreadLocal reference
------ One thing is that some of the code's using real tabs set to show as 4 spaces, while my setup was using all spaces. What's the project standard, as it's easy enough to set Intellij to whichever?
Gwyn
On Wed, 1 Dec 2004 11:53:34 +0100, Juergen Donnerstag
<[EMAIL PROTECTED]> wrote:
If I then choose the DisplayTags exmaples, then the ExampleExport, then Export to CSS, I get a Null Pointer Exception showing up in HttpRequestCycle.
What seems to be happeing is that the rendering for 0.exportCSV does a setPage(), then invokes the 'linkClicked" method on the ExportLink component, which does a setPage(null), but when the rendering gets to the 'finally' block in RequestCycle, there's no check in either the call to or the implementaton of 'detatchModels(page)', which gives the NPE.
A check, either in the call or the body, fixes the problem, but I'm not sure if it's the real problem or showing something else up - comments?
ExportLink.linkClicked() sets the page == null in order to prevent any page rendering (the response contains the export data not the page rendered). This worked fine so far. RequestCycle.render(), especially the finally block however depends since recently on a valid page. Currently I don't see other means to return the export data and prevent rendering the page. Thus, the finally block should not rely on a valid page variable.
regards
Juergen
-------------------------------------------------------
SF email is sponsored by - The IT Product Guide
Read honest & candid reviews on hundreds of IT Products from real users.
Discover which products truly live up to the hype. Start reading now. http://productguide.itmanagersjournal.com/
_______________________________________________
Wicket-develop mailing list
[EMAIL PROTECTED]
https://lists.sourceforge.net/lists/listinfo/wicket-develop
-------------------------------------------------------
SF email is sponsored by - The IT Product Guide
Read honest & candid reviews on hundreds of IT Products from real users.
Discover which products truly live up to the hype. Start reading now. http://productguide.itmanagersjournal.com/
_______________________________________________
Wicket-develop mailing list
[EMAIL PROTECTED]
https://lists.sourceforge.net/lists/listinfo/wicket-develop
