You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@struts.apache.org by "zhouyanming (JIRA)" <ji...@apache.org> on 2010/05/21 07:29:16 UTC
[jira] Created: (WW-3451) [FreemarkerManager] don't put
ATTR_SESSION_MODEL into session
[FreemarkerManager] don't put ATTR_SESSION_MODEL into session
----------------------------------------------------------------
Key: WW-3451
URL: https://issues.apache.org/jira/browse/WW-3451
Project: Struts 2
Issue Type: Bug
Reporter: zhouyanming
Priority: Critical
Attachments: patch.txt
put ATTR_SESSION_MODEL into session is useless in current code, and harmful for distributed system
and remove some other useless code,please see patch.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (WW-3451) [FreemarkerManager] don't put
ATTR_SESSION_MODEL into session
Posted by "Christian Wolfgang Stone (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/WW-3451?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12870381#action_12870381 ]
Christian Wolfgang Stone commented on WW-3451:
----------------------------------------------
First, while I am happy to have a solution that doesn't pollute the session, I take exception to your choice of words. I hope you can communicate like an adult.
There already is a filter, the Struts Execute Filter, that releases control. There is also a Servlet, the FreemarkerDecoratorServlet that handles external decoration. I was not suggesting that you code a new filter, but look at the filter in place and what it calls (which is what you are already modifying), or the servlet that needs the information so you can see a better fix. If you understood the architecture, and are calling the Struts 2 architecture *crappy*, I think that is a different matter. The fact that Struts 2 uses a filter is not *crappy*. I would consider Struts 2 mature, and while I would code things a bit differently, it works quite well. So if you are stating that Struts 2 is crappy because it uses a filter (actually several filters), I think you could be better off saying that the web.xml file is getting polluted and we can do better. So you either look at the struts-execute filter (which is what I recommend), or the servlet (which is the majority of what I wrote). Anyways, it seems like you are putting a whole lot more energy into a bad fix instead of asking perhaps why certain things work the way they do so a fix can be made quickly.
I do agree with the change:
model.put(KEY_SESSION, sessionModel);
It is a redundancy from the code a few lines before and that should be a good fix.
Now it has been a while, but as I remember a new request is generated by struts under certain conditions (struts tags perhaps... and others)? The end result was that important request information was being dumped that freemarker really needed to have. This information was more than just the tags, but the results of the users actions, such as adding request objects... these things are necessary, and care should be taken to maintain the state.
I think a more elegant solution would include copying the model to (and possibly from if the changes would not modify the model object) the new request objects as they are created within Struts, and make them available to the freemarker Servlet in the request at the end. This way you wouldn't need to store anything in the session object, or worry about passing it forward where it could be serialized. I would still look at extending the model wrapper and making it so it won't be serialized, but I would first look carefully at the Freemarker code and see why they rely on serialization.
I do agree that in a cluster system heavyweight object can be evil in a session. I am happy that you are addressing this. However, looking for a better fix is much better than reintroducing lots of bugs...
> [FreemarkerManager] don't put ATTR_SESSION_MODEL into session
> ----------------------------------------------------------------
>
> Key: WW-3451
> URL: https://issues.apache.org/jira/browse/WW-3451
> Project: Struts 2
> Issue Type: Bug
> Reporter: zhouyanming
> Priority: Critical
> Attachments: patch.txt
>
>
> put ATTR_SESSION_MODEL into session is useless in current code, and harmful for distributed system
> and remove some other useless code,please see patch.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (WW-3451) [FreemarkerManager] don't put
ATTR_SESSION_MODEL into session
Posted by "zhouyanming (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/WW-3451?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
zhouyanming updated WW-3451:
----------------------------
Attachment: patch.txt
> [FreemarkerManager] don't put ATTR_SESSION_MODEL into session
> ----------------------------------------------------------------
>
> Key: WW-3451
> URL: https://issues.apache.org/jira/browse/WW-3451
> Project: Struts 2
> Issue Type: Bug
> Reporter: zhouyanming
> Priority: Critical
> Attachments: patch.txt
>
>
> put ATTR_SESSION_MODEL into session is useless in current code, and harmful for distributed system
> and remove some other useless code,please see patch.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (WW-3451) [FreemarkerManager] don't put
ATTR_SESSION_MODEL into session
Posted by "Christian Wolfgang Stone (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/WW-3451?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12869967#action_12869967 ]
Christian Wolfgang Stone commented on WW-3451:
----------------------------------------------
I would be very careful when applying this patch, as it looks like it breaks the features requested in Sitemesh. When you say "useless in current code", I presume you mean that you have devised another method of caching the model so that Freemarker can access Struts request and session stack in Sitemesh?
Can you explain why this breaks other features?
While I didn't write the no-caching code you wish to remove, why is it redundant? I presume you are inferring that elsewhere in the code those headers are being set?
> [FreemarkerManager] don't put ATTR_SESSION_MODEL into session
> ----------------------------------------------------------------
>
> Key: WW-3451
> URL: https://issues.apache.org/jira/browse/WW-3451
> Project: Struts 2
> Issue Type: Bug
> Reporter: zhouyanming
> Priority: Critical
> Attachments: patch.txt
>
>
> put ATTR_SESSION_MODEL into session is useless in current code, and harmful for distributed system
> and remove some other useless code,please see patch.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (WW-3451) [FreemarkerManager] don't put
ATTR_SESSION_MODEL into session
Posted by "zhouyanming (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/WW-3451?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12870413#action_12870413 ]
zhouyanming commented on WW-3451:
---------------------------------
sorry for misunderstanding your suggestion,I thought you are suggesting user write their own filter to do that,but reuse Struts Execute Filter have a bad smell too,because freemarker view is not core function of struts2.
[quote]
I would first look carefully at the Freemarker code and see why they rely on serialization.
[/quote]
it doesn't matter with freemarker,session replication will cause serialization.
objects in session often are inconsistent in a cluster system,for example
node1 session.setAttribute("user",user);
node2 will receive a event triggerred by setAttribute method and replicate user object.
then node1 modify object's state like user.setPassword("newpassword");
but in node2,user still have an old state unless you call session.setAttribute("user",user) everytime after modifiy user.
so put object into session is evil.
you think new a HttpSessionHashModel every request could impact performance,have you do some test to prove that?
my patch is identical with trunk code from function aspect,you can run unit tests to assert it.
and identical from performance aspect due to your mistyping.
BTW:I don't think struts2 is crappy,I like it.
> [FreemarkerManager] don't put ATTR_SESSION_MODEL into session
> ----------------------------------------------------------------
>
> Key: WW-3451
> URL: https://issues.apache.org/jira/browse/WW-3451
> Project: Struts 2
> Issue Type: Bug
> Reporter: zhouyanming
> Priority: Critical
> Attachments: patch.txt
>
>
> put ATTR_SESSION_MODEL into session is useless in current code, and harmful for distributed system
> and remove some other useless code,please see patch.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (WW-3451) [FreemarkerManager] don't put
ATTR_SESSION_MODEL into session
Posted by "zhouyanming (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/WW-3451?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12870368#action_12870368 ]
zhouyanming commented on WW-3451:
---------------------------------
I presume you are coping code from line 315
ServletContextHashModel servletContextModel = (ServletContextHashModel) servletContext.getAttribute(ATTR_APPLICATION_MODEL);
if (servletContextModel == null) {
// first try a JSP support servlet. If it fails, default to the servlet.
GenericServlet servlet = JspSupportServlet.jspSupportServlet;
if (servlet != null) {
servletContextModel = new ServletContextHashModel(servlet, wrapper);
servletContext.setAttribute(ATTR_APPLICATION_MODEL, servletContextModel);
} else {
servletContextModel = new ServletContextHashModel(servletContext, wrapper);
servletContext.setAttribute(ATTR_APPLICATION_MODEL, servletContextModel);
}
TaglibFactory taglibs = new TaglibFactory(servletContext);
servletContext.setAttribute(ATTR_JSP_TAGLIBS_MODEL, taglibs);
}
model.put(KEY_APPLICATION, servletContextModel);
model.putUnlistedModel(KEY_APPLICATION_PRIVATE, servletContextModel);
but your code :
HttpSessionHashModel sessionModel;
if (session != null) {
sessionModel = (HttpSessionHashModel) session.getAttribute(ATTR_SESSION_MODEL);
if (sessionModel == null) {
sessionModel = new HttpSessionHashModel(session, wrapper);
session.setAttribute(ATTR_SESSION_MODEL, sessionModel);
}
model.put(KEY_SESSION, new HttpSessionHashModel(session, wrapper));
} else {
// no session means no attributes ???
// model.put(KEY_SESSION_MODEL, new SimpleHash());
}
I think you want
model.put(KEY_SESSION, sessionModel);
not
model.put(KEY_SESSION, new HttpSessionHashModel(session, wrapper));
you recommended to use a filter remove such unnecessary object is a crappy suggestion,nobody want be responsible for this immature design.
I redeclare put unnecessary object into session is evil,particularly in distributed or cluster system.
> [FreemarkerManager] don't put ATTR_SESSION_MODEL into session
> ----------------------------------------------------------------
>
> Key: WW-3451
> URL: https://issues.apache.org/jira/browse/WW-3451
> Project: Struts 2
> Issue Type: Bug
> Reporter: zhouyanming
> Priority: Critical
> Attachments: patch.txt
>
>
> put ATTR_SESSION_MODEL into session is useless in current code, and harmful for distributed system
> and remove some other useless code,please see patch.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (WW-3451) [FreemarkerManager] don't put
ATTR_SESSION_MODEL into session
Posted by "zhouyanming (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/WW-3451?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12870341#action_12870341 ]
zhouyanming commented on WW-3451:
---------------------------------
HttpSession session = request.getSession(false);
HttpSessionHashModel sessionModel;
if (session != null) {
sessionModel = (HttpSessionHashModel) session.getAttribute(ATTR_SESSION_MODEL);
if (sessionModel == null) {
sessionModel = new HttpSessionHashModel(session, wrapper);
session.setAttribute(ATTR_SESSION_MODEL, sessionModel);
}
model.put(KEY_SESSION, new HttpSessionHashModel(session, wrapper));
}
here sessionModel is put into session,but didn't use it elsewhere,everytime it will new HttpSessionHashModel(session, wrapper)
put objects that user didn't known is a bad smell,and new a instance is not so expensive in comparison to serialization and deserialization
> [FreemarkerManager] don't put ATTR_SESSION_MODEL into session
> ----------------------------------------------------------------
>
> Key: WW-3451
> URL: https://issues.apache.org/jira/browse/WW-3451
> Project: Struts 2
> Issue Type: Bug
> Reporter: zhouyanming
> Priority: Critical
> Attachments: patch.txt
>
>
> put ATTR_SESSION_MODEL into session is useless in current code, and harmful for distributed system
> and remove some other useless code,please see patch.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (WW-3451) [FreemarkerManager] don't put
ATTR_SESSION_MODEL into session
Posted by "Christian Wolfgang Stone (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/WW-3451?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12870355#action_12870355 ]
Christian Wolfgang Stone commented on WW-3451:
----------------------------------------------
Oh I disagree...
First, it will be reused by freemarker often with decorators. It maintains the state you set in the freemarker view with the decorator, including the knowledge of struts actions, etc.. Also, Freemarker disallows the use of ".XXX" notation, so these parameters cannot be accessed from Freemarker views, but the underlying model can access them. Users won't be able to access or set such parameters in the session object. So it remains safe. This model supports the Freemarker method of storing and retaining underlying model states, so it is also consistent and will likely be easier to migrate to future Freemarker configurations.
The smell comment is pretty funny. I don't even have a comment for it besides the humor.
It seems that you have a better solution that requires a careful coding with regards to Freemarker, the next Freemarker client (which I recommend you research before you code your changes), and the Struts dispatcher. I would love to see it! However, implementing a patch that breaks functionality without also implementing new code that addresses the threatened features is a very bad idea.
If the only issue you are concerned with is the serialization and de-serialization accross servers, I recommend looking at the servlet filter and implementing code that removes the session object at the completion of the request, or implement a robust request handler that captures the request state before the dispatcher releases control to the filters so that it can be reconstructed when the filters pick it up. OR, perhaps overwrite the serializable functionality of the HttpSessionHashModel with a new object that leaves a blank object and test for that. This way if the session is serialized, you have a near zero length object that won't affect performance.
I look forward to seeing the new patch you submit and testing it on my servers. I take it you are volunteering to implement the work for the changes you need...
> [FreemarkerManager] don't put ATTR_SESSION_MODEL into session
> ----------------------------------------------------------------
>
> Key: WW-3451
> URL: https://issues.apache.org/jira/browse/WW-3451
> Project: Struts 2
> Issue Type: Bug
> Reporter: zhouyanming
> Priority: Critical
> Attachments: patch.txt
>
>
> put ATTR_SESSION_MODEL into session is useless in current code, and harmful for distributed system
> and remove some other useless code,please see patch.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.