My vote is 1. Small key size used to store views into session 2. use the hashCode of the viewId 3. keep the code as is
Leonardo 2012/11/14 Leonardo Uribe <[email protected]>: > 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
