myfaces-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Leonardo Uribe <lu4...@gmail.com>
Subject [VOTE] Design of MyFaces server side state saving algorithm and related considerations
Date Thu, 15 Nov 2012 01:17:57 GMT
Hi

Recently in

https://issues.apache.org/jira/browse/MYFACES-3638

We have found some problems related to how ServerSideStateCacheImpl should
work. In few words, the code in that class deals with all logic
related to server
side state saving, so it is a critical point in MyFaces Core.

We need some help to get this out, so a vote over this considerations
is necessary.
It is a long story, so I divided the mail into sections, so if you
already know the
background, you can skip to the important parts.

--- BACKGROUND ---

In 1.1.x and 1.2.x, this code was in this location:

http://svn.apache.org/repos/asf/myfaces/core/branches/1.2.x/impl/src/main/java/org/apache/myfaces/application/jsp/JspStateManagerImpl.java

The idea is when a view needs to be saved, a key is created (SerializedViewKey)
using a session scope counter and the full viewId. This key is later
serialized,
encrypted, tampered (add a mac code), and base64 encoded to be stored in
javax.faces.ViewState hidden field. Also, the key is used in a internal map to
store the calculated view state that will be used later to restore the view.

In MYFACES-3117, we saw the need to align MyFaces state saving
implementation with the one in the spec, and fix some problems related to the
implementation of ResponseStateManager. Some changes were done to
isolate responsibilities, and the code in JspStateManagerImpl was moved to
ServerSideStateCacheImpl.

In MYFACES-3563, another round of improvements were done (changes done
in 2.1.9 / 2.0.15), with the intention of remove some old code that
does not have
sense anymore with the new implementation and improve server side state
saving, trying to mimize the overhead associated with store a view.

One of the changes done was this:

https://issues.apache.org/jira/browse/MYFACES-3568

[perf] use viewId hashCode() instead full viewId on server side state saving
SerializedViewKey

The justification of this improvement is this:

"... In SerializedViewKey, the current code uses a counter and the full
viewId as key to retrieve or save the state into session.

But store the full viewId is not really necessary. The counter itself gives
uniqueness inside the session, and the viewId is just a way to check if
the state to restore is the right one. In other words, given the probability
of found two valid viewIds in an application with the same hashCode is
astronomical and the fact that there is a counter that identify in an unique
way a view state session token, it is reasonable to store into the state
only the hashCode, and when the view is restored compare the viewId
hashCode with the stored one into the state.

This change will reduce the size required by SerializedViewKey. ..."

Originally, the idea of use both a viewId and a counter to derive a key was
done because the old algorithm relies on the viewId to ensure uniqueness
of the key. That's how is working now in 1.2.x / 1.1.x and it is working
just fine.

But based on the improvements done in MYFACES-3568 and MYFACES-3117
it was realized that only the counter is required to ensure key uniqueness,
and the viewId comparison is not necessary. For example, Mojarra allows
to use a counter or a random number to generate valid keys (see
com.sun.faces.generateUniqueServerStateIds web config parameter).

--- DISCUSSION POINTS ---

In MYFACES-3638, some changes were done, trying to simplify the code, but
ignoring some considerations already accepted. In few words the controversial
points are:

1. In MYFACES-3568, the following private internal classes extending from
an abstract class called SerializedViewKey:

- IntIntSerializedViewKey
- IntByteArraySerializedViewKey
- ReferenceSerializedViewKey

The idea is serialize an Object is more expensive than serialize a
primitive type.
So, using IntIntSerializedViewKey and IntByteArraySerializedViewKey in
production stage will lead to a smaller session size compared to use the old
implementation of SerializedViewKey, with requires to store the full viewId.

The advantages of this improvement are:

- Small session size
- Since the key is small it will be faster to encrypt and encode it.

See this link for details about this:

http://www.cs.virginia.edu/kim/publicity/pldi09tutorials/memory-efficient-java-tutorial.pdf

but the claimed disadvantage is that three classes for do the same that can be
done just once, just increase complexity without need.

2. There is still doubts about if the full viewId should be stored in
the key or not. The
evidence suggest that it is not necessary, but if that so, it is worth
to store the hashCode
of the viewId? In theory, the probability of have two real viewIds
with the same hashCode
is astronomical, so even if it is not the same as an equality
comparison, it helps to
decide if a state can be applied to a view or not.

3. Since there are too many classes inside ServerSideStateCacheImpl,
it is better
practice to put those classes outside the class, to have a better
layout of the code.
Even if sounds a good idea to do it, the current design using private
static classes
ensures that it is not possible to create keys outside the algorithm
and override
the values inside the internal map that holds the view state. For example, with
Java 2 security it is possible to ensure that code cannot be executed
using reflection
and so on. Putting the classes outside the class makes harder to control that.

Note that myfaces code already uses an strict checkstyle policy.

-- VOTE --

Please vote for each point:

1. Small key size used to store views into session vs go back to the old way.
2. use the hashCode of the viewId vs store the full viewId and do an
equals comparison vs remove it from the key.
3. Split ServerSideStateCacheImpl into many classes vs keep the code as is.

Please note that in case we can't get to an agreement, according to
the rules, since the
changes in MYFACES-3563 were committed long time ago without any objections,
lazy consensus applies[1], and the changes proposed recently needs to
be reverted.

regards,

Leonardo Uribe

[1] http://community.apache.org/committers/lazyConsensus.html

Mime
View raw message