myfaces-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Gerhard Petracek <gerhard.petra...@gmail.com>
Subject Re: CODI: @ViewScoped implementation possibly flawed.
Date Sun, 18 Nov 2012 19:46:22 GMT
hi radu,

please have a look at [1].
if you have further questions, you are welcome to ask them and we will
improve [1] based on them.

regards,
gerhard

[1] http://incubator.apache.org/deltaspike/community.html

http://www.irian.at

Your JSF/JavaEE powerhouse -
JavaEE Consulting, Development and
Courses in English and German

Professional Support for Apache MyFaces



2012/11/18 Radu Creanga <rdcrng@gmail.com>

> Thanks to everyone for consideration.
>
> > Yes, the Apache DeltaSpike project is the joined effort in this area.
> That's good to hear because CDI together with all the extensions
> already rocks and this may just make it even better.
>
> > we imported it in deltaspike already -> please file an issue there [1].
> Sure thing, on it.
>
> > + you are welcome to join the effort.
> I may need a mentor to get started since I've never had the privilege
> to contribute to any OSS project before. I promise to get out of your
> hair ASAP. Anyone willing to volunteer? :)
>
> Radu Creanga
>
>
> On Sun, Nov 18, 2012 at 1:54 PM, Gerhard Petracek
> <gerhard.petracek@gmail.com> wrote:
> > hi radu,
> >
> > we imported it in deltaspike already -> please file an issue there [1].
> > + you are welcome to join the effort.
> >
> > changes to the imported implementations will get reviewed and if it makes
> > sense we will merged them back to codi (esp. important fixes).
> >
> > regards,
> > gerhard
> >
> > [1] https://issues.apache.org/jira/browse/DELTASPIKE
> >
> > http://www.irian.at
> >
> > Your JSF/JavaEE powerhouse -
> > JavaEE Consulting, Development and
> > Courses in English and German
> >
> > Professional Support for Apache MyFaces
> >
> >
> >
> >
> > 2012/11/18 Mark Struberg <struberg@yahoo.de>
> >>
> >> Hi Radu!
> >>
> >> Thansk for the feedback. Yes, observing session events would be a
> possible
> >> workaround for broken JSF containers/spec issues.
> >>
> >> > Did CODI join forces with Seam?
> >> Yes, the Apache DeltaSpike project is the joined effort in this area.
> For
> >> the @ViewScoped implementation this doesn't have any impact because I
> >> (together with the help Jakob and Gerhard)  wrote both of them ;)
> >>
> >>
> >>
> >> LieGrue,
> >> strub
> >>
> >>
> >>
> >> ----- Original Message -----
> >> > From: Radu Creanga <rdcrng@gmail.com>
> >> > To: MyFaces Development <dev@myfaces.apache.org>
> >> > Cc:
> >> > Sent: Sunday, November 18, 2012 6:28 PM
> >> > Subject: Re: CODI: @ViewScoped implementation possibly flawed.
> >> >
> >> > 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,  <ssilvert@redhat.com> 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 <rdcrng@gmail.com>
> >> >>>>  To: dev@myfaces.apache.org
> >> >>>>  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
> >> >>>>
> >> >>>>
> >> >>>>
> >> >>
> >> >
> >
> >
>

Mime
View raw message