javax.servlet.http.HttpSession methods such as getAttribute(), getValue(), getAttributeNames(), getValueNames(), etc, throw an IllegalStateException if called on a session that has been invalidated. So, with the following code in a JSP page: <% session.invalidate(); Object obj = pageContext.findAttribute("foo"); %> An IllegalStateException is thrown because pageContext.findAttribute() eventually calls session.getAttribute() on a session that has been invalidated. The session that has been invalidated should simply be ignored when a method needs to process the various scopes (page, request, session, application). This impacts the following methods in PageContextImpl: public int getAttributesScope(final String name) which calls -> private int doGetAttributeScope(String name); public Object findAttribute(final String name) which calls -> private Object doFindAttribute(String name); public void removeAttribute(final String name) which calls -> private void doRemoveAttribute(String name); The fix is to catch IllegalStateException and ignore it when processing the attribute in session scope. The code then simply follows through to process application scope. No need to worry about setAttribute() because it is always invoked on a specific scope, and the spec already states that java.lang.IllegalStateException must be thrown when called on an invalidated session. pageContext.setAttribute("foo", "value of foo", PageContext.SESSION_SCOPE); java.lang.IllegalStateException - if the scope is PageContext.SESSION_SCOPE but the page that was requested does not participate in a session or the session has been invalidated. --------------------------------------------------------------------------- At the same time, a fix should be done to method "doRemoveAttribute(String name)" where a try/catch block for Exception appears unnecessary. private void doRemoveAttribute(String name){ try { removeAttribute(name, PAGE_SCOPE); removeAttribute(name, REQUEST_SCOPE); if( session != null ) { try { removeAttribute(name, SESSION_SCOPE); } catch (IllegalStateException ex) { // Session has been invalidated. // Ignore and fall through to application scope. } } removeAttribute(name, APPLICATION_SCOPE); } catch (Exception ex) { // we remove as much as we can, and // simply ignore possible exceptions } } Here is a full analysis: Starting with 'removeAttribute(final String name)' - we check for null and throw NPE if necessary - we call doRemoveAttribute(name) doRemoveAttribute(name) - we call removeAttribute(name, scope) for each scope removeAttribute(final String name, final int scope) - this calls doRemoveAttribute(name, scope) doRemoveAttribute(name, scope) - page scope: attributes.remove -> won't throw an Exception - request scope: request.removeAttribute -> no documented Exception thrown - session scope: throws IllegalStateException if session is null - app scope: context.removeAttribute -> no documented Exception thrown A null value for name is already checked in removeAttribute(final String name) and we throw NPE. So this situation (removing an attr from page or request scope throwing an NPE) won't happen. In doRemoveAttribute(name), we already check on session != null before calling removeAttribute(name, SESSION_SCOPE). So there normally is no IllegalStateException thrown (except for the invalidated case). When removing an attribute from application (i.e., ServletContext) scope, any registered listeners will be notified, but the code that does that (see appserv-webtier/src/java/org/apache/catalina/core/ApplicationContext. removeAttribute()) already catches any Throwable that a listener may throw. The try/catch block is therefore unnecessary. Moreover, if any of the removal actions from the different scopes could have thrown an exception, each of them would have needed to be wrapped inside their own try/catch, so as to ensure that an exception in one scope does not cause any of the subsequent removals to be bypassed. doRemoveAttribute(String name) has therefore been modified as follows: private void doRemoveAttribute(String name){ removeAttribute(name, PAGE_SCOPE); removeAttribute(name, REQUEST_SCOPE); if( session != null ) { try { removeAttribute(name, SESSION_SCOPE); } catch (IllegalStateException ex) { // Session has been invalidated. // Ignore and fall through to application scope. } } removeAttribute(name, APPLICATION_SCOPE); } -------------------------- Changes done on glassfish. ymmv on jasper for the diffs. --- PageContextImpl.java 9 Dec 2005 18:54:30 -0000 1.7 +++ PageContextImpl.java 16 Dec 2005 00:11:21 -0000 1.8 @@ -452,8 +452,13 @@ return REQUEST_SCOPE; if (session != null) { + try { if (session.getAttribute(name) != null) return SESSION_SCOPE; + } catch (IllegalStateException ex) { + // Session has been invalidated. + // Ignore and fall through to application scope. + } } if (context.getAttribute(name) != null) @@ -495,9 +500,14 @@ return o; if (session != null) { + try { o = session.getAttribute(name); - if (o != null) - return o; + } catch (IllegalStateException ex) { + // Session has been invalidated. + // Ignore and fall through to application scope. + } + + if (o != null) return o; } return context.getAttribute(name); @@ -559,19 +569,18 @@ } } - private void doRemoveAttribute(String name){ - try { removeAttribute(name, PAGE_SCOPE); removeAttribute(name, REQUEST_SCOPE); if( session != null ) { + try { removeAttribute(name, SESSION_SCOPE); + } catch (IllegalStateException ex) { + // Session has been invalidated. + // Ignore and fall through to application scope. } - removeAttribute(name, APPLICATION_SCOPE); - } catch (Exception ex) { - // we remove as much as we can, and - // simply ignore possible exceptions } + removeAttribute(name, APPLICATION_SCOPE); } public JspWriter getOut() {
*** Bug 37699 has been marked as a duplicate of this bug. ***
In the future, please attach patches from GlassFish rather than inlining them. As it is, I can't do anything about this since I've already seen the patch, so I'm tainted from an IP persective. Before this patch can be accepted, we need to know that your submission is covered by a CCLA. Otherwise, Sun owns the code, so it can't be incorporated into Tomcat.
Hi William, Thanks for looking into this so quickly. The situation is the same as the one you encountered with Kin-Man on bug 35351: >> Just to be clear, is this patch is still covered by your CCLA? In English, is >> this a grant from glassfish to that ASF? >> > This is just a patch, and there is no license or copyright, so YES. Let me know if you need anything else.
(In reply to comment #3) > Hi William, > Thanks for looking into this so quickly. > The situation is the same as the one you encountered > with Kin-Man on bug 35351: > >> Just to be clear, is this patch is still covered by your CCLA? In English, is > >> this a grant from glassfish to that ASF? > >> > > This is just a patch, and there is no license or copyright, so YES. > Let me know if you need anything else. I accepted Kin-Man's patch based on the fact that he is a Tomcat committer, and as such should have a CCLA on file with the ASF. If that is still valid, than that means that Sun has trusted him with deciding about donating Sun code to the ASF. Were I to find out that Kin-Man's CCLA has been revoked, I would revert the commit in a heart-beat :). IANAL, but from all of the ASF lists that I have followed, the above reasoning doesn't even hold a drop of water. So again I ask if you have a CCLA on file so that the ASF can accept your donation of Sun's code? The patch is nice, so I'm hoping that you say yes ;-).
With very minor style changes, your patch has been committed to SVN trunk, and will appear in 5.5.15. Thanks much!
It seems the problem still exists in the Tomcat 6.0.18. Moreover, I took a look at the PageContextImpl.java class and I do not see there any fixes related to this bug. I browsed svn repository and it looks like this commit has reverted the fix: http://svn.apache.org/viewvc/tomcat/jasper/tc6.0.x/src/share/org/apache/jasper/runtime/PageContextImpl.java?view=log Is it supposed to be so? Or fix was reverted by mistake?
Moving to Tomcat 6
Thanks for the report and the research. This has been fixed in trunk and proposed for 6.0.x
This has been fixed in 6.0.x and will be included in 6.0.20 onwards.