Leo, I'm ok with a revert. But not to your version but to the original JspStateManagerImpl!
Really, this currently feels so overcomplicated without giving much benefit. It would have been perfectly enough to remove the viewId String and replace it with a XORShift random value if you don't trust the sequencer. I know one goal was to abstract out the state between client and server. But that doesn't mean that we end up with Object, Object and do hardcoded upcasts as done right now in the code. This is no improvement over the original code. The windowId change would have been 30 LOC btw. LieGrue, strub ----- Original Message ----- > From: Leonardo Uribe <[email protected]> > To: MyFaces Development <[email protected]>; Mark Struberg > <[email protected]> > Cc: > Sent: Thursday, November 15, 2012 8:44 PM > Subject: Re: StateSaving review > > Hi > > 2012/11/15 Mark Struberg <[email protected]>: >> Btw, I still do not see where the trick for storing a separate state list > for each browser tab is hidden. >> That was the goal of all the refactoring initially. Where is that done? >> > > Mark, months ago we created 2.1.x-client-window branch: > > http://svn.apache.org/repos/asf/myfaces/core/branches/2.1.x-client-window/ > > This branch is the same as 2.1.x, but only with the changes proposed > long time ago for JSF 2.2 client window. > > It has what you need to have a state list for each browser tab. > The only thing you need to provide is a ClientWindow implementation > and that's it. The state saving code already has included the client-window > concept. I also fixed Flash scope to use client window. > It is also available in a maver repo: > > https://repository.apache.org/content/repositories/snapshots/org/apache/myfaces/core/myfaces-bundle/ > > Let's make a deal. Move your changes to that branch, let me apply my fix > in trunk and do a release. When your changes are ready, we can discuss > them and backport from 2.1.x-client-window to trunk. Does that sound > good for you? > > regards, > > Leonardo Uribe > > >> LieGrue, >> strub >> >> >> >> ----- Original Message ----- >>> From: Mark Struberg <[email protected]> >>> To: MyFaces Development <[email protected]> >>> Cc: >>> Sent: Thursday, November 15, 2012 6:32 PM >>> Subject: Re: StateSaving review >>> >>> 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 >>>> >>> >
