Session scoped beans and activeViewMaps not cleaned up when PortletSession expires or is invalidated

ren-zhijun-oracle commented 11 years ago

When running in the context of a request, Mojarra calls FacesContext.getCurrentInstance().getExternalContext.getSessionMap() in order to get/set session attributes. This provides the portlet bridge with the ability to get/set attributes using the PortletSession, which is a layer of abstraction on top of the HttpSession. But when a session expires, the com.sun.faces.application.WebappLifecycleListener.sessionDestroyed(HttpSessionEvent) method calls HttpSessionEvent.getSession() in order to get/set session attributes. This causes a memory leak when running in a portlet environment, because the portlet bridge is not consulted. Specifically, @SessionScoped managed-beans and the "com.sun.faces.activeViewMaps" attribute are not cleaned up.

The good news is that Section PLT.18.3 of the Portlet 2.0 Specification titled "Binding Attributes into a Session" requires that PortletSession attribute names be namespaced/prefixed with the "javax.portlet.p.?" pattern when they are stored in the underlying HttpSession. This would enable Mojarra to find the session attributes, so that cleanup can take place correctly during session expiration/invalidation.

Affected Versions

[2.1.17]

ren-zhijun-oracle commented 6 years ago

Issue Imported From: https://github.com/javaserverfaces/mojarra/issues/2695
Original Issue Raised By:@javaserverfaces
Original Issue Assigned To: @manfredriem
Original Issue Closed By:@javaserverfaces

ren-zhijun-oracle commented 11 years ago

@javaserverfaces Commented Reported by ngriffin7a

ren-zhijun-oracle commented 11 years ago

@javaserverfaces Commented ngriffin7a said:

Index: jsf-ri/src/main/java/com/sun/faces/mgbean/BeanManager.java
===================================================================
--- jsf-ri/src/main/java/com/sun/faces/mgbean/BeanManager.java  (revision 11386)
+++ jsf-ri/src/main/java/com/sun/faces/mgbean/BeanManager.java  (working copy)
@@ -198,8 +198,31 @@

      public boolean isManaged(String name) {

-        return (managedBeans != null && managedBeans.containsKey(name));
-
+       boolean managed = false;
+       
+       if (managedBeans != null) {
+           
+           if (managedBeans.containsKey(name)) {
+               managed = true;
+           }
+           else {
+               // Section PLT.18.3 of the Portlet 2.0 Specification titled +               // "Binding Attributes into a Session" requires that +              // PortletSession attribute names be namespaced/prefixed with +             // the "javax.portlet.p.<ID>?" pattern. In order to determine +             // if the specified name is a SessionScoped managed-bean, it +              // is necessary to first strip the pattern from it. +               if ((name != null) && name.startsWith("javax.portlet.p.")) {
+                   int pos = name.indexOf("?");
+                   if (pos > 0) {
+                       String attributeName = name.substring(pos+1);
+                       managed = managedBeans.containsKey(attributeName);
+                   }
+               }
+           }
+       }
+       
+       return managed;
     }

Index: jsf-ri/src/main/java/com/sun/faces/application/WebappLifecycleListener.java
===================================================================
--- jsf-ri/src/main/java/com/sun/faces/application/WebappLifecycleListener.java (revision 11386)
+++ jsf-ri/src/main/java/com/sun/faces/application/WebappLifecycleListener.java (working copy)
@@ -202,18 +202,34 @@

         /*
          * When the session gets destroyed we need to make sure that each view
-         * scope that was still active cleans up properly.
+         * scope that was still active cleans up properly. Note that in a portlet
+         * environment, there could be a separate "com.sun.faces.activeViewMaps"
+         * attribute for each portlet. Therefore it is necessary to iterate through
+         * each of the attributes.
          */
-        Map<String, Object> activeViewMaps = (Map<String, Object>) session.getAttribute("com.sun.faces.activeViewMaps");
-        if (activeViewMaps != null) {
-            Iterator<Object> activeViewMapsIterator = activeViewMaps.values().iterator();
-            while(activeViewMapsIterator.hasNext()) {
-Map<String, Object> viewMap = (Map<String, Object>) activeViewMapsIterator.next();
-BeanManager beanManager = applicationAssociate.getBeanManager();
-Iterator<Entry<String, Object>> viewEntries = viewMap.entrySet().iterator();
-while(viewEntries.hasNext()) {
-    Entry<String, Object> entry = viewEntries.next();
-    beanManager.destroy(entry.getKey(), entry.getValue());
+        for (Enumeration e = session.getAttributeNames(); e.hasMoreElements(); ) {
+            String attributeName = (String)e.nextElement();
+            if (attributeName != null) {
+
+// Section PLT.18.3 of the Portlet 2.0 Specification titled +// "Binding Attributes into a Session" requires that +// PortletSession attribute names be namespaced/prefixed with +// the "javax.portlet.p.<ID>?" pattern. +if (attributeName.equals("com.sun.faces.activeViewMaps") ||
+        (attributeName.startsWith("javax.portlet.p.") && attributeName.endsWith("com.sun.faces.activeViewMaps"))) {
+   Map<String, Object> activeViewMaps = (Map<String, Object>) session.getAttribute(attributeName);
+    if (activeViewMaps != null) {
+        Iterator<Object> activeViewMapsIterator = activeViewMaps.values().iterator();
+        while(activeViewMapsIterator.hasNext()) {
+            Map<String, Object> viewMap = (Map<String, Object>) activeViewMapsIterator.next();
+            BeanManager beanManager = applicationAssociate.getBeanManager();
+            Iterator<Entry<String, Object>> viewEntries = viewMap.entrySet().iterator();
+            while(viewEntries.hasNext()) {
+Entry<String, Object> entry = viewEntries.next();
+beanManager.destroy(entry.getKey(), entry.getValue());
+            }
+        }
+    }
 }
             }
         }

ren-zhijun-oracle commented 11 years ago

@javaserverfaces Commented @edburns said: I've applied the patch and am running our automated tests. Manfred Riem will have some comments shortly, as relating to #2692.

ren-zhijun-oracle commented 11 years ago

@javaserverfaces Commented @manfredriem said: Neil,

Can you see if you can use your own sessionDestroyed listener that gets invoked when the session gets destroyed and execute the following in it?

For each portlet attribute: 1. Remove the prefixed attribute out of the session (it should trigger a session attribute removed). 2. Stick the attribute with a non-prefixed name back into the session. 3. Remove the non-prefixed attribute out of the session (it should trigger a session attribute removed).

Then we should be able to trap the session attribute removed event and call the beanManager destroy method. That way if we would have to rework code in the future your code is unaffected.

Can you verify if that would work?

ren-zhijun-oracle commented 11 years ago

@javaserverfaces Commented ssilvert said: I think it will work as long as WebappLifecycleListener.attributeRemoved() handles activeViewMaps in the same manner as WebappLifecycleListener.sessionDestroyed().

The problem I see is that this logic would need to replicated by every portlet bridge implementation. It would need to go into the portlet bridge spec. I'd rather see us go with Neil's solution because it will work with all portlet bridges that are in use today.

We can address it at the spec level later.

ren-zhijun-oracle commented 11 years ago

@javaserverfaces Commented ngriffin7a said: I agree with Stan – the trouble is that we would would have replicate the fix in a listener in every portlet bridge implementation. And in Servlet 2.5 containers, I think it would impose an obligation on the portlet developer to add a portlet-bridge-specific in the WEB-INF/web.xml descriptor.

Since these are Mojarra-managed session attributes, it would be nice if it could be handled by Mojarra.

ren-zhijun-oracle commented 11 years ago

@javaserverfaces Commented @manfredriem said: I would like Neil to test if it can be done using a listener. Since the attribute renaming is not part of the JSF spec.

And if that works, then we would proceed with the following:

1. File an issue to have the Portlet bridge spec be updated to state it needs to do this (to fix it at the bridge level). 2. Let me know so I can update this issue with a pointer to that Portlet bridge spec issue. 3. Integrate this patch to the current runtime with a reference to this issue (to make it work now). 4. File a spec issue referencing this issue that describes what a compliant JSF impl needs to do.

If it does not work, I will investigate further to see what needs to be done to get it working with the approach described earlier.

ren-zhijun-oracle commented 11 years ago

@javaserverfaces Commented ngriffin7a said: There are no plans to rev the portlet bridge spec at this time. One idea for the JSF 2.2 spec that could solve this, would be to acquire a non-request-based FacesContext from the FacesContextFactory, or perhaps the ExternalContext from the ExternalContextFactory that could be used in the Mojarra HttpSessionListener. That way, Mojarra would consult ExternalContext.getSessionMap() and the bridge could be involved the normal way.

ren-zhijun-oracle commented 11 years ago

@javaserverfaces Commented @edburns said: I have created [JAVASERVERFACES_SPEC_PUBLIC-1155](https://java.net/jira/browse/JAVASERVERFACES_SPEC_PUBLIC-1155 "Require FacesContext.getCurrentInstance() to work at Servlet "SessionDestroyed" event time") "Require FacesContext.getCurrentInstance() to work at Servlet "SessionDestroyed" event time" to capture the feature request. I cannot consider it for 2.2 due to time pressure.

Neil and Stan, I see your point that every portlet bridge impl will have to do the same thing. I counter by pointing out that every JSF impl will have to do the same thing if we go with simply applying your initial patch. I'm not opposed to applying the patch, but I do think that Manfred's suggestion deserves trying out. If it's not in the spec, it does seem better to keep it "closer to home", as Manfred's suggestion would provide for.

ren-zhijun-oracle commented 11 years ago

@javaserverfaces Commented @edburns said: Neil, can you please consider putting a TLD in the bridge that has a

com.foo.bridge.MyBridgeListenerImpl

And in there you can listen for the event without having to require the end developer to declare a listener.

This is the approach we've been using to bootstrap the JSF impl since 1.0.

ren-zhijun-oracle commented 11 years ago

@javaserverfaces Commented ssilvert said: I think you can go ahead and close [JAVASERVERFACES_SPEC_PUBLIC-1155](https://java.net/jira/browse/JAVASERVERFACES_SPEC_PUBLIC-1155 "Require FacesContext.getCurrentInstance() to work at Servlet "SessionDestroyed" event time"). Just getting FacesContext at SessionDestroyed time won't help the problem. At that point, you no longer have a portlet context. So using ExternalContext to manipulate the SessionMaps of N portlets is meaningless. The usual write-through to the portlet session won't work.

All you have a SessionDestroyed time is the HttpSession with attributes and values from multiple portlets.

I think the spec change that will be needed is to have something like WebappLifecycleListener defined in the spec as part of the JSF API. Then a bridge impl can extend or replace the default impl. I don't have the specifics worked out in my head though.

ren-zhijun-oracle commented 11 years ago

@javaserverfaces Commented ngriffin7a said: Point well taken about every JSF impl needing to do the same thing. For more info, see: https://issues.apache.org/jira/browse/MYFACES-3675

Thanks for the hint about the TLD. I will try it on the portlet bridge side and report back.

ren-zhijun-oracle commented 11 years ago

@javaserverfaces Commented ngriffin7a said: Hi Manfred,

I tried the following steps using an HttpSessionListener with Liferay Faces Bridge. I tested with GlassFish 3.1.2, JBoss AS7, and Tomcat 7:

For each portlet attribute: 1. Remove the prefixed attribute out of the session (it should trigger a session attribute removed). 2. Stick the attribute with a non-prefixed name back into the session. 3. Remove the non-prefixed attribute out of the session (it should trigger a session attribute removed).

Good News:

The Mojarra WebappLifecycleListener.attributeRemoved(HttpSessionBindingEvent) method was called for step #1 and step #3.
Mojarra successfully cleaned up a session scope bean after step#3

Bad News:

On all app servers, the Mojarra WebappLifecycleListener.sessionDestroyed(HttpSessionEvent) method was called before the bridge's listener. This means that Mojarra did not have an opportunity to cleanup the "com.sun.faces.activeViewMaps" attribute. Not sure if there is a way to specify order when registering listeners via TLD files?

Additional Thoughts:

1) Bridges would need to limit attribute removal to just SessionScoped managed-beans. Bridges would have to check to see if the attribute values have an @ManagedBean annotation, and pre-scan WEB-INF/faces-config for all elements for a matching FQCN.

2) We would have to continue testng on Jetty, Resin, Geronimo, WebLogic, WebSphere, and JOnAS in order to make sure that HttpSessionAttributeListener.remove() methods are called properly durring sessionDestroyed events.

3) What to do about the "com.sun.faces.activeViewMaps" attribute problem? If the bridge were to pro-actively remove it, then perhaps Mojarra could perform cleanup with the WebappLifecycleListener.attributeRemoved(HttpSessionBindingEvent) method?

Thanks for your help,

Neil

ren-zhijun-oracle commented 11 years ago

@javaserverfaces Commented @manfredriem said: Hi Neil,

The activeViewMaps should be empty after step #3 has runs its course with respect to all the portlet renamed managed session beans. Then once the session goes out of scope the reference to these maps should be garbage collected, so I do not think they are going to be an issue

1) Since we are talking about attributes that are stored the session scope I don't think it does do any harm to remove these when you are receiving the session destroyed event?

Note you should not need to rename the 'com.sun.faces.activeViewMaps' attribute.

ren-zhijun-oracle commented 11 years ago

@javaserverfaces Commented ngriffin7a said: Hi Manfred,

If the cleanup processing in WebappLifecycleListener.sessionDestroyed(HttpSessionEvent) method does not process the "com.sun.faces.activeViewMaps" attribute, then JBoss AS7 will experience a memory leak and hold onto references for @ViewScoped managed-beans. Stan can correct me, but I believe that the Mojarra BeanManager.destroy(String, Object) method must be called on each of the @ViewScoped managed-beans so that the JBoss InjectionProvider can have a chance to execute.

If Mojarra were to perform cleanup processing for "com.sun.faces.activeViewMaps" in both WebappLifecycleListener.sessionDestroyed(HttpSessionEvent) and WebappLifecycleListener.attributeRemoved(HttpSessionBindingEvent), then we could implement your proposal in the bridge. I can post another patch if that would help.

Thanks,

Neil

ren-zhijun-oracle commented 11 years ago

@javaserverfaces Commented @manfredriem said: Hi Neil,

OK with respect to those if you do not rename the com.sun.faces.application.view.activeViewMaps attribute at all, the regular processing will take care of cleaning up those references as part of the sessionDestroyed event so they would be covered.

ren-zhijun-oracle commented 11 years ago

@javaserverfaces Commented ngriffin7a said: Hi Manfred,

The problem is that the Mojarra WebappLifecycleListener.sessionDestroyed(HttpSessionEvent) method tries to do this:

Map<String, Object> activeViewMaps = (Map<String, Object>) session.getAttribute("com.sun.faces.application.view.activeViewMaps");

In a portlet environment, the attribute name is "javax.portlet.p.XXX?com.sun.faces.application.view.activeViewMaps" and so the value of activeViewMaps is null. Because it's null, the cleanup processing does not take place.

Neil

ren-zhijun-oracle commented 11 years ago

@javaserverfaces Commented @manfredriem said: What I am saying do not rename that attribute at all. Let it behave normally. And then you should be fine.

ren-zhijun-oracle commented 11 years ago

@javaserverfaces Commented ngriffin7a said: Hi Manfred,

Understood – whether the bridge renames the attribute or not doesn't make a difference. The WebappLifecycleListener.sessionDestroyed(HttpSessionEvent) method always fails to perform cleanup processing in a portlet environment.

Thanks,

Neil

ren-zhijun-oracle commented 11 years ago

@javaserverfaces Commented @manfredriem said: Can we connect on Skype? I already send out the request

ren-zhijun-oracle commented 11 years ago

@javaserverfaces Commented ngriffin7a said: This issue can be closed – thanks to the refactoring in #2692, the new ViewScopeManager can be called via reflection from portlet bridges during session expiration which cleans up the ViewScoped memory leaks.

ren-zhijun-oracle commented 11 years ago

@javaserverfaces Commented @manfredriem said: Closing as per reporter

ren-zhijun-oracle commented 11 years ago

@javaserverfaces Commented ngriffin7a said: Thanks Ed, Manfred, and Stan for all your help!

ren-zhijun-oracle commented 11 years ago

@javaserverfaces Commented ngriffin7a said: Corresponding issue in Liferay issue tracker: http://issues.liferay.com/browse/FACES-1470

ren-zhijun-oracle commented 11 years ago

@javaserverfaces Commented Issue-Links: is related to JAVASERVERFACES-2688 is related to JAVASERVERFACES_SPEC_PUBLIC-1155

ren-zhijun-oracle commented 7 years ago

@javaserverfaces Commented This issue was imported from java.net JIRA JAVASERVERFACES-2691

ren-zhijun-oracle commented 11 years ago

@javaserverfaces Commented Marked as works as designed on Wednesday, January 23rd 2013, 6:55:17 am

eclipse-ee4j / mojarra

Session scoped beans and activeViewMaps not cleaned up when PortletSession expires or is invalidated #2695

Affected Versions