Stan, I reviewed the spec changes. What they do is monitor session, session attribute, context, context attributes, context, and context attribute events then properly fire PreDestroyViewMapEvents. This change would actually fix CODI's implementation as it stand since this is the event that CODI's implementation monitors.
Mark, "It actually has nothing to do with CODI nor CDI." - please allow me to argue otherwise. The CDI spec says that Context implementations are responsible for destroying beans, see here (http://docs.jboss.org/cdi/spec/1.0/html_single/#contextual). Whoever wrote the CODI implementation thought that listening for PreDestroyViewMapEvents would be enough to ensure that, which I don't blame them, look at the comments: /** * We get PreDestroyViewMapEvent events from the JSF servlet and destroy our contextual * instances. This should (theoretically!) also get fired if the webapp closes, so there * should be no need to manually track all view scopes and destroy them at a shutdown. * * @see javax.faces.event.SystemEventListener#processEvent(javax.faces.event.SystemEvent) */ But, this event does not get fired, therefore a better implementation would monitor for at least HttpSessionEvents and destroy the instances it creates. Also, you say that it's a similar problem with every passivation capable bean. Sure, in the case that the beans would be passivated or replicated across the domain, I wouldn't blame the context for not being able to destroy them. But, there are plenty of instances when view scoped beans never get passivated, just like sessions. And the current implementation doesn't even destroy those. Now, it is not possible (to the best of my knowledge) to register for session events directly from the Contextual implementation, since by then the servlet would be initialized. Therefore, writing a CDI extension that propagates the servlet and session lifecycle events to CDI, then having the Contextual implementation observe those events in order to properly destroy instance it creates would be one solution. I understand that this may be overkill since the new JSF spec will fix this in a few months. Also, from what I understand CODI does not currently have an extension that monitors for servlet related events so it would be quite a lot of work to get there. Did CODI join forces with Seam? If so, it may be worth bringing this up to them. I'd be happy to work on it, since I already have to anyway for my internal project. Pointers would be appreciated. Radu Creanga On Fri, Nov 16, 2012 at 12:14 PM, <[email protected]> wrote: > Problems with @ViewScoped and @PreDestroy have been identified in the > spec and a fix is slated for JSF 2.2: > http://java.net/jira/browse/JAVASERVERFACES_SPEC_PUBLIC-905 > > You guys might want to comment on it. > > Stan > > On 11/16/2012 9:50 AM, Mark Struberg wrote: >> >> Hi Radu! >> >> Congratulations, you spotted a very deeply buried problem :) It actually has >> nothing to do with CODI nor CDI. The problem is similar with every >> passivation capable (==Serializable) bean: @PreDestroy is not 100% >> guaranteed to get fired. >> >> I'm not sure if you really did hit a bug in the 2 JSF impls or a systematic >> consequence of serializable objects I like to explain now: >> >> >> Think about a @ViewScoped bean. The @PreDestroy invokes a user.logout() for >> example. Now you navigate away from the page and the @ViewScoped bean gets >> closed. Then press the browser back button and you will have the bean again >> (restored from the viewstate map). That might lead to @PreDestroy getting >> invoked multiple times. >> >> >> Now another case: a user opens a page.xhtml in his browser. A @ViewScoped >> bean gets called and if ClientSideStateSaving is activated it gets >> serialized and stored inside the html page. The user just goes away or >> closes his browser and this bean will never get the @PreDestroy being called. >> >> Something similar happens with all @SessionScoped or session related beans >> if a server shuts down and passivates it's sessions. If there is a >> redeployment inbetween which is not binary compatible, then those beans will >> never get resurrected and never get properly destroyed because of that. >> >> It get's even worse if you think about Session replication to multiple >> nodes. On which node should the @PreDestroy being called? And at what time? >> >> >> Again: not sure if you did hit any of the 'natural' reasons or if it is >> really a bug. If it's a bug then not in CODI but the JSF impls. >> >> >> >> LieGrue, >> strub >> >>> ________________________________ >>> From: Radu Creanga <[email protected]> >>> To: [email protected] >>> Sent: Friday, November 16, 2012 3:25 PM >>> Subject: CODI: @ViewScoped implementation possibly flawed. >>> >>> >>> Dear CODI devs, >>> >>> >>> Please allow me to bring to your attention a possible flaw in the >>> implementation of @ViewScoped. I apologize in advance if this is not the >>> case. >>> >>> >>> The class >>> org.apache.myfaces.extensions.cdi.jsf2.impl.scope.view.ViewScopedContext >>> provides the life-cycle for @ViewScoped annotated objects. An >>> implementation of Context, according to the documentation: >>> >>> >>> "[...] the context object is responsible for destroying any contextual >>> instance it creates by passing the instance to Contextual.destroy(Object, >>> CreationalContext). A destroyed instance must not subsequently be returned >>> by get(). The context object must pass the same instance of >>> CreationalContext to Contextual.destroy() that it passed to >>> Contextual.create() when it created the instance." >>> >>> >>> The ViewScopedContext implementation in question attempts to achieve this >>> requirement by: >>> 1. implementing javax.faces.event.SystemEventListener; >>> 2. registering itself for javax.faces.event.PreDestroyViewMapEvent; >>> 3. destroying Contextual instances upon receiving those events. >>> >>> >>> The problem is that PreDestroyViewMapEvent don't seem to be fired. >>> Intuitively, these events should be fired every time UIViewRoot objects are >>> destroyed, but after spending hours trying to get at least one of them to >>> fire I have seen none, neither on MyFaces, nor on Mojarra JSF >>> implementations. I expected to see these events fired at least on session >>> termination or web-app shut-down, but no luck. >>> >>> >>> The documentation for javax.faces.event.PreDestroyViewMapEvent states: >>> >>> >>> "This event must be published by a call to >>> Application.publishEvent(javax.faces.context.FacesContext, java.lang.Class, >>> java.lang.Object) when the clear method is called on the map returned from >>> UIViewRoot#getViewMap." >>> >>> >>> Is clear()required to be called anywhere? The only reference in the >>> documentation where I could find such a requirement is at >>> FacesContext.setViewRoot(): >>> >>> >>> "If the current UIViewRoot is non-null, and calling equals() on the >>> argument root, passing the current UIViewRoot returns false, the clear >>> method must be called on the Map returned from UIViewRoot#getViewMap." >>> >>> >>> If this is the only place where clear()on the view map is called (I suspect >>> it is since I could not get any of these events to fire), which is only >>> during the Invoke Application phase of the request processing lifecycle and >>> during the Restore View phase of the request processing lifecycle >>> (especially when a new root component is created), then most if not all >>> Contextual instances created by >>> org.apache.myfaces.extensions.cdi.jsf2.impl.scope.view.ViewScopedContextare >>> not disposed properly. >>> >>> >>> I was lead to this while trying to implement my own @ViewScoped context and >>> could not verify that the instances I create are ever destroyed. This lead >>> me to post a question on stackoverflow.com, from where I was suggested I >>> take a look at the CODI implementation. From what I can tell, both >>> implementations do not properly dispose of contextual objects they create, >>> while offering the end-user behaviour they advertise. >>> >>> >>> Hopefully, this is useful insight and I would be honoured to be permitted >>> to get involved in coming up with a solution, assuming a problem exists. >>> >>> Radu Creanga >>> >>> >>> >
