I did further reviews: Currently the CounterKeyFactory needs some random to be unique (according to Leo) and the RandomKeyFactory needs a counter to be unique.
Does that ring a bell? That stuff is completely useless! Foooooolks wake up, let's provide ONE factory which is waterproof. That would be much better than having 15++ classes full of half baken/broken algorithms. Proposal: Get rid of all that KeyFactory stuff again and have one GOOD algorithm: Counter + XORShift Random. The KeyFactory stuff got only introduced in 2.1.9 and is crashing in production. LieGrue, strub PS: don't take it personal, but from looking at the code I see clearly what happens if someone works on a stuff alone without reviews. This always leads to deficits, regardless how good one is. ----- Original Message ----- > From: Leonardo Uribe <[email protected]> > To: MyFaces Development <[email protected]>; Mark Struberg > <[email protected]> > Cc: > Sent: Thursday, November 15, 2012 6:20 PM > Subject: Re: StateSaving review > > Hi > > 2012/11/15 Mark Struberg <[email protected]>: >> I'm currently reviewing all this area. It seems that we have quite some > stuff to improve. >> >> a.) just a gut feeling yet, but my tummy tells me that we have to review > our key generator/storage strategies. Too complicated or too badly > documented. > At least they are not self describing. > > It's a complex algorithm. No documentation so far, because it is still > work in progress. > >> >> b.) candidate 1: CounterKeyFactory. If we like to prevent reboot clashes > then we might add another int which contains a random value. Think about > having > a Server with a single page right now. Click on it a few times, then restart > myfaces and issue a few requests to the same page and go back in your browser > history. Proposal: instead of the viewId we should add a random number. >> > > Since the counter is also stored into session, the last counter values > is not lost. > >> c.) a general one. We might introduce an own Random which either uses > java.util.concurrent.ThreadLocalRandom for java 7++ or the old Random impl. >> ThreadLocalRandom has a _much_ better performance on servers! Or we just > use a simple XORShift which is surely good enough for most cases and performs > like hell. The spreading of XORShift is better than the standard Java > algorithm > even ... >> > > It could work. > >> d.) KeyFactory looks a bit overengineered. The return type is either > Integer or byte [] but the encoded value is always represented as String. >> > > The key is encrypted and base64 encoded, but that's done in > HtmlResponseStateManager.writeViewStateField. The change only has > sense if we move the responsibility of encrypt to > ServerSideStateCacheImpl/ClientSideStateCacheImpl. My opinion is it is > better to keep it as is. > >> e.) Instead of trashing the Session with setAttribute and synchronized > blocks we should rather store an AtomicInteger. This is perfectly fine now as > we > do not support java 1.4 any longer, right? >> > > The synchronized blocks are over the SerializedViewCollection, which > is unique per session, so it does not suppose any overhead (tested > with YourKit profiler). But performance tests shows that the > additional calls to session map impose an small overhead (the ideal is > minimize those calls). Since the counter is stored per session, there > is no chance of key clashing. One option could be something like the > trick used in Flash Scope. An AtomicLong from a random seed, but > anyway it is necessary to change the algorithm for check if there is a > key clashing, generates another key. > >> Just a few random ideas... >> > > MS>> another one: > MS>> All that stuff has nothing to do with the RenderKit and should go > into org.apache.myfaces.application.viewstate. > MS>> wdyt? > > That stuff is used by > org.apache.myfaces.renderkit.html.HtmlResponseStateManager, so in > theory is related to the renderkit (because ResponseStateManager is > renderkit dependant), but it is an application scope concept. Maybe > org.apache.myfaces.renderkit.application.viewstate. has more sense to > me. > > MS>> Also the following classes are related and should imo get moved > to this new package: > MS>> * StateCache > MS>> * StateCacheFactory > > Yes, a better name > > MS>> * StateManagerImpl > > No, StateManagerImpl deals with the logic related to calculate the > state to a view, StateCache deals with the storage of the view (maybe > a better name is StateStorage, for example > ServerSideStateStorageImpl/ClientSideStateStorageImpl? > > regards, > > Leonardo > >> LieGrue, >> strub >
