1. I almost committed the code without the old constructor, but since it
is (in some way) accessible to code outside Stripes, I decided the
better of it. I figured we could remove it for 1.5. I did updated it to
only use StripesRequestWrapper.hashCode(). But really, it wouldn't
bother me a bit to remove that constructor since really nobody should be
using it.
2. I freaked out and thought way too much about the synchronization
stuff. What I was trying to do by syncing on that String is make sure I
was syncing on something that only this code would sync on. I figured
maybe there could be other code that might sync on the session so I
didn't want to do that. So I just chose something that was specific to
the code I was working on. I tried to come up with something better, but
at the time I couldn't. FlashScope.class might be a good choice. It
should be noted that if we add any static synchronized methods in the
future they will be syncing on the class, too.
3. Based on the performance difference I saw on Linux between seeding a
new Random and reusing an old one, I decided that reusing was the way to
go. But I have no data to show that reusing is better when taking
synchronization issues into account. I'm fine with using a new Random
each time if that's what you prefer. We're talking about really small
numbers either way :)
4. I've never done a merge with svn. I was just working with it when
your message arrived. It looks to me like I can check out a working copy
of the 1.4.x branch and then do svn merge -r BASE:HEAD <file>. Is that
right?
Tim Fennell wrote:
> Hey Ben,
>
> First of all, this looks great! Good work. I have a couple of
> comments/questions:
>
> 1. The old (now deprecated) constructor. While I don't think we
> should remove it in the 1.4.3 release....it should be totally safe to
> just delete it in the trunk. There are no calls to it in Stripes, and
> since it's protected and there's no way to easily use a subclass of
> FlashScope, nobody else should be using it either. Do you agree?
>
> 2. At FlashScope:350 you syncrhonize on a public static final String
> in StripesConstants. I've never done this myself, but I'm wondering
> if it's safe? The compiler has a tendency to inline references to
> public static final Strings, and I wonder if that might end up
> negating the sync'ing, or if the compiler is smart enough to not
> inline (or intern the String) in cases like this. Might just be
> safer to sync on FlashScope.class instead?
>
> 3. I'm still a little leery of the use of a static Random for
> generation of the keys, that then has to be synchronized over.
> Admittedly the contents of the sync {} are short and quick, and this
> isn't something that should be called that much (one per request
> maximum), but it still concerns me. I'm probably worried about
> nothing, but I just wonder if there isn't a strategy that would be as
> good and not involve JVM-wide syncing.
>
> 4. You're gonna merge this to the 1.4.x branch right? ;)
>
> -t
>
> On Mar 30, 2007, at 12:09 AM, [EMAIL PROTECTED] wrote:
>
>
>> Revision: 500
>> http://svn.sourceforge.net/stripes/?rev=500&view=rev
>> Author: bengunter
>> Date: 2007-03-29 21:09:09 -0700 (Thu, 29 Mar 2007)
>>
>> Log Message:
>> -----------
>> Resolved STS-319: Request and response objects should be released
>> when using flash scope.
>> At the end of each request, any ActionBeans in the current flash
>> scope have their
>> contexts' request and response objects replaced with objects that
>> are safe to use on the
>> next request.
>>
>> - Added FlashRequest and FlashResponseInvocationHandler classes
>>
>> - Deprecated FlashScope(HttpServletRequest) in favor of
>> FlashScope(HttpServletRequest, Integer)
>>
>> - The key for the current flash scope for a request is now stored
>> as a request attribute
>>
>> - Some cleanup code was moved from StripesFilter.flashOutbound() to
>> FlashScope.requestComplete()
>>
>> - Added additional flash scope tests, including tests for the new
>> request and response
>>
>> Modified Paths:
>> --------------
>> trunk/stripes/src/net/sourceforge/stripes/controller/
>> FlashScope.java
>> trunk/stripes/src/net/sourceforge/stripes/controller/
>> StripesConstants.java
>> trunk/stripes/src/net/sourceforge/stripes/controller/
>> StripesFilter.java
>> trunk/tests/src/net/sourceforge/stripes/controller/
>> FlashScopeTests.java
>>
>> Added Paths:
>> -----------
>> trunk/stripes/src/net/sourceforge/stripes/controller/
>> FlashRequest.java
>> trunk/stripes/src/net/sourceforge/stripes/controller/
>> FlashResponseInvocationHandler.java
>>
>
>
> -------------------------------------------------------------------------
> Take Surveys. Earn Cash. Influence the Future of IT
> Join SourceForge.net's Techsay panel and you'll get the chance to share your
> opinions on IT & business topics through brief surveys-and earn cash
> http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
> _______________________________________________
> Stripes-development mailing list
> [email protected]
> https://lists.sourceforge.net/lists/listinfo/stripes-development
>
-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
_______________________________________________
Stripes-development mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/stripes-development
