Leo, NO, please read again: >>> 1. Small key size used to store views into session vs go back to the > old way. >> >> Take a safe approach. Adding just the hashCode doesn't make it more > safe. Either the full viewId or get rid of it completely if not needed. >> >> >>> 2. use the hashCode of the viewId vs store the full viewId and do an >>> equals comparison vs remove it from the key. >> IF the viewId is important than we must do the equals on the viewId in > addition. >> >> >>> 3. Split ServerSideStateCacheImpl into many classes vs keep the code as > is. >> Split into own package. It's really not maintainable. Btw, I like the > old StateManagerImpl name better than the 'Cache'. It is much more to > the point imo.
And now for your 'turning around my words' > 1. Go back to the old way, just one class. NO definitely not, see my answer 3 > 2. The viewId is not important --> get rid of it. NO, FIRST check if it is important, then either get rid or fix it. > 3. Split ServerSideStateCacheImpl into many classes YES, and clean it up, document it, etc. LieGrue, strub ----- Original Message ----- > From: Leonardo Uribe <[email protected]> > To: MyFaces Development <[email protected]>; Mark Struberg > <[email protected]> > Cc: > Sent: Thursday, November 15, 2012 7:17 AM > Subject: Re: [VOTE] Design of MyFaces server side state saving algorithm and > related considerations > > So your vote is: > > 1. Go back to the old way, just one class. > 2. The viewId is not important --> get rid of it. > 3. Split ServerSideStateCacheImpl into many classes > > Let's try to be concrete, so we can get to a constructive conclusion. > > I think both positions are clear, I don't want to waste my time in > futile discussions, it is pointless. I respect and appreciate your > arguments, but let the people interested take a position, otherwise > this will go to nowhere. This is not like politics, we need to take > action and the sooner the better. > > 2012/11/15 Mark Struberg <[email protected]>: >> >>> 1. Small key size used to store views into session vs go back to the > old way. >> >> Take a safe approach. Adding just the hashCode doesn't make it more > safe. Either the full viewId or get rid of it completely if not needed. >> >> >>> 2. use the hashCode of the viewId vs store the full viewId and do an >>> equals comparison vs remove it from the key. >> IF the viewId is important than we must do the equals on the viewId in > addition. >> >> >>> 3. Split ServerSideStateCacheImpl into many classes vs keep the code as > is. >> Split into own package. It's really not maintainable. Btw, I like the > old StateManagerImpl name better than the 'Cache'. It is much more to > the point imo. >> >> >> >> Now for the details: >> Most of your arguments are unrelated. E.g. Serializing a String is exactly > as cheap as serializing a byte[]. There is really zero difference. > > As a last comment, the int stored in replacement of the viewId using > hashCode() method is just 4 bytes vs a String with the full viewId, > which has a lenght of more than 10 or 20 characters, and that could be > 44 bytes, maybe more? If the session is not serialized the effect is > similar. There is a difference and is not zero or near zero. It is > enough significant to take some time and try to minimize it. Does it > helps the hashCode of viewId? yes, because with only 4 bytes we can be > sure with a very high probability that the state can be applied to the > view. It is strictly necessary? no. > > To show it in practice this is the generated markup for > javax.faces.ViewState right now using the default encryption > algorithm: > > <input type="hidden" name="javax.faces.ViewState" > id="javax.faces.ViewState" value="rO0ABXQAATE=" /> > > This is what we had before (myfaces 2.1.7): > > <input type="hidden" name="javax.faces.ViewState" > id="javax.faces.ViewState" > value="rO0ABXVyABNbTGphdmEubGFuZy5PYmplY3Q7kM5YnxBzKWwCAAB4cAAAAAJ1cQB+AAAAAAACdAABMXB0AAsvaG9tZS54aHRtbA==" > /> > > Which one is better? It is worth to do it? which one will be faster to > encrypt? > >> Also the impls didn't change. It's now just much more complicated > than before. I would have expected to do the obvious improvements like > getting > rid of the synchronized block by simply using an AtomicInteger as counter. > THOSE > things bring performance. >> >>> 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. >> This is preventing exactly nothing. UIViewRoot has a public interface to > change that. Any JavaScript code on the client can change it. >> You can prevent both if a SM is in place and you can perfectly invoke both > if not. >> It only makes it harder to maintain and extend the code. >> >> >> LieGrue, >> strub >> >> >> >> >> ----- Original Message ----- >>> From: Leonardo Uribe <[email protected]> >>> To: MyFaces Development <[email protected]> >>> Cc: >>> Sent: Thursday, November 15, 2012 2:17 AM >>> Subject: [VOTE] Design of MyFaces server side state saving algorithm > and related considerations >>> >>> 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 >>> >
