> 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. Cool. Then let's just delete it from the trunk once your merge is done.
> 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 :) I'm gonna play a little today and see if/when the contention becomes an issue. > 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? The way I do this is that I have a copy of the branch checked out too, go over there and execute (in the case of your commit): svn merge -r499:500 https://svn.sourceforge.net/svnroot/stripes/trunk And that'll grab difference between those two versions from the URL supplied, and then apply them to the working copy. It /usually/ does a really good job - the only problem is if the classes have diverged enough it can't figure out how to apply the patch, but I doubt that'll happen here. -t > > 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 ------------------------------------------------------------------------- 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
