myfaces-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Leonardo Uribe <lu4...@gmail.com>
Subject Re: StateSaving review
Date Thu, 15 Nov 2012 18:40:34 GMT
Hi

2012/11/15 Mark Struberg <struberg@yahoo.de>:
> 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!
>

A random generator does not ensure uniqueness, only a counter do that.
The random works as a secret that only the server knows. Restore a
view only works if the secret match with the one stored into session

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

Ok.

> The KeyFactory stuff got only introduced in 2.1.9 and is crashing in production.
>

Just because there is 1 bug that happens in a corner case doesn't
meant applications using 2.1.9 don't work. I have not been able to
reproduce the bug mentioned. I suppose that's the reason why the TCK
tests and other tests one founded any problem. If you check the code,
you can see some junit tests in place (ServerSideStateCacheTest,
ClientSideStateCacheTest and a lot more).

I don't have any doubt the fix proposed by me works, which only
changes 1 or 2 lines.

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

No worries, this is just work, nothing personal to me. I review and
test everything that is committed in myfaces core, and I have proof of
that.

>
> ----- Original Message -----
>> From: Leonardo Uribe <lu4242@gmail.com>
>> To: MyFaces Development <dev@myfaces.apache.org>; Mark Struberg <struberg@yahoo.de>
>> Cc:
>> Sent: Thursday, November 15, 2012 6:20 PM
>> Subject: Re: StateSaving review
>>
>> Hi
>>
>> 2012/11/15 Mark Struberg <struberg@yahoo.de>:
>>>  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
>>

Mime
View raw message