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 >>> >>
