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
