This is an omnibus reply to a big private email thread that I think
would better be discussed on the Mojarra email list. I've Bcc'd the
people that started the thread and who may not be on
dev_at_javaserverfaces.dev.java.net
>>>>> On Thu, 18 Mar 2010 22:36:33 +0100, Dünnenberger Hanspeter said:
HD> tried to contact you on the chat - but actually that might work
HD> better by email.
UIComponentBase.getFacesContext() says
HD> // PENDING(edburns): we can't use the cache ivar because we
HD> // don't always know when to clear it. For example, in the
HD> // "save state in server" case, the UIComponent instances stick
HD> // around between requests, yielding stale facesContext
HD> // references. If there was some way to clear the facesContext
HD> // cache ivar for each node in the tree *after* the
HD> // render-response phase, then we could keep a cache ivar. As
HD> // it is now, we must always use the Thread Local Storage
HD> // solution.
HD> return FacesContext.getCurrentInstance();
HD> In the comment you asked a way to clear the cache variable - well,
HD> will UIComponentBase.saveState() allways be called - also with new
HD> partial state saving?
AS> I think that we should explore this possibility further since this might
AS> provide a solution that would work with Trinidad's view root caching.
AS> Note that even when the Trinidad view root caching optimization is used,
AS> we still need to save state so that the state is available in the
AS> session for replication/fail over purposes.
[...]
>>>>> On Fri, 19 Mar 2010 10:10:18 +0100, "Marinschek Martin (KCBC 111)" said:
MM> I absolutely agree with Hanspeter that this should go. There is two
MM> issues why I think Mojarra uses FacesContext.getCurrentInstance():
MM> 1) in the beginnning, Mojarra didn't really execute
MM> saveState/restoreState, it just reused the component tree for the
MM> next request - but this has been fixed way back
Yes, as Hanspeter mentioned in his first email.
MM> 2) in the portlet environment, there are two requests, an action
MM> request and a render request. The portlet bridge might (wrongly)
MM> decide to reuse the component
[...]
>>>>> On Wed, 31 Mar 2010 15:25:00 +0200, "Marinschek Martin (KCBC 111)" <martin.marinschek_at_credit-suisse.com> said:
[...]
MM> and we could actually save 2-3% of application performance if this
MM> was cached.
[...]
MM> Our solution for this now would be that the faces-context is cached,
MM> but that the tag-handler resets the faces-context information
MM> whenever it creates or updates the UIComponent instance.
This would most likely be new public API, as you know.
MM> Now, this should work even for the dumb portlet bridges. However, I
MM> have just learned that Trinidad actually caches the full component
MM> tree, and just re-uses it on the next request.
MM> @Andy, will the tag-handlers then be reapplied, so that our strategy
MM> works, or is not even this happening? In this case, we might run
MM> into problems with this approach. Do you have a solution for this?
And what about for JSP?
AS> Another safer option might be for UIComponentBase to cache the
AS> FacesContext for shorter time periods. For example,
AS> UIComponentBase.encodeBegin() could cache, UIComponentBase.encodeEnd()
AS> could clear the cache and UIComponent.getFacesContext() could first
AS> check for a cached instance, and if not found fall back on
AS> FacesContext.getCurrentInstance(). That seems like a much safer
AS> solution that should provide a decent improvement - assuming that most
AS> calls to UIComponent.getFacesContext() are coming from the
AS> UIComponentBase.AttributeMaps.get() during rendering.
I like this conservative approach very much as well.
MM> yes - so why don't do it at all three locations:
MM> 1) saveState
MM> 2) ComponentHandler.apply for creating a new component and
MM> 3) ComponentHandler.apply for finding an existing component
MM> that should be pretty safe!
B> component tree before we cleared out the references. One way to
B> make the presence of stale references more apparent would be to move
B> the FacesContext instance into an invalid state when disconnecting
B> it from the ThreadLocal.
We already have this in Mojarra. Every public method calls
assertNotRealeased() first thing. This method does not assert(), it
throws an ISE if it's released. Now, it might be possible a
FacesContext could be not released, but still be divorced from its
ThreadLocal, but I don't see how.
B> Trinidad should be cleaning out the transient subtrees during
B> state saving (in case the page author adding any properties
B> that he doesn't expect to see on the next request), so I think
B> we should be OK there.
MM> ok - so my suggestion looks good
Martin, your 1) 2) 3) suggestion you mean?
MM> I believe that when a component is state-saved and it has a binding
MM> attribute, we should call the binding attribute and pass null
MM> through to the setter. With this, we might at least be able to
MM> prevent the greatest damage. This would make a lot more sense than
MM> the current behaviour of leaving this to the user to get right.
Yes, I think this is safe, assuming that the binding attribute gets
restored correctly, which it should.
MM> @Ed: do you want me to open issues for this? do you agree with this
MM> outcome? We can discuss some more on the EG-list of course...
I have done so
<
https://javaserverfaces.dev.java.net/issues/show_bug.cgi?id=1599>.
Ed
--
| edward.burns_at_oracle.com | office: 408 884 9519 OR x31640
| homepage: | http://ridingthecrest.com/