This would be an imprecise use of synchronization for the HashMap. The atomic set of operations that needs synchronized is:
1. Lookup 2. If there, return 3. If not, create and add new one Without synchronization around that entire set of operations, there is a race condition when two threads get to step 3 simultaneously for the same Action and then they both add an instance of the same Action type (last one wins in terms of which instance the HashMap ends up keeping). Also, recall that the double-check pattern is unsafe in Java given its current synchronzation and memory access semantics (http://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html). As an optimization for this case, where we know that a second or third Action getting created is not a horrible event that could cause program incorrectness, we can say we'll live with a smaller synchronization block for concurrency purposes. But that should be an explicit decision on a case-by-case basis w/ copious comments in the code. FWIW... Donnie > -----Original Message----- > From: [EMAIL PROTECTED] [mailto:[EMAIL PROTECTED]] > Sent: Monday, April 15, 2002 8:29 PM > To: [EMAIL PROTECTED] > Subject: DO NOT REPLY [Bug 8138] New: - The synchronized keyword is used > too early in the processActionCreate() method of the RequestProcessor > class > > > DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG > RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT > <http://nagoya.apache.org/bugzilla/show_bug.cgi?id=8138>. > ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND > INSERTED IN THE BUG DATABASE. > > http://nagoya.apache.org/bugzilla/show_bug.cgi?id=8138 > > The synchronized keyword is used too early in the > processActionCreate() method of the RequestProcessor class > > Summary: The synchronized keyword is used too early in the > processActionCreate() method of the RequestProcessor > class > Product: Struts > Version: Nightly Build > Platform: Other > OS/Version: Other > Status: NEW > Severity: Normal > Priority: Other > Component: Controller > AssignedTo: [EMAIL PROTECTED] > ReportedBy: [EMAIL PROTECTED] > > > In the processActionCreate() method, inside the RequestProcessor > class, the > synchronized keyword is used when an Action instances is either > retrieved or > created. The synchronized keyword is a dangerous operation to > use, especially > in a web application, since it blocks all other threads. It > should be should > only at the time that it needs to. > > The processActionCreate() method uses it before it determines > whether it needs > to create a new one or not. The get from the HashMap should not be > synchronized, since it's a read-only operation. If the instance > is not found in > the HashMap, then synchronized should be used to do the create of the new > instance. This may improve scalability slightly also. > > Chuck > > p.s. While you're in there fixing this, can you change the type > of the actions > object to a Map. Obviously the implementation should be a > HashMap, but you > should almost always declare variables with a super interface, if > you can. > Map's, List's, etc... Not a concreate class like HashMap, > ArrayList, etc... > > -- > To unsubscribe, e-mail: <mailto:[EMAIL PROTECTED]> For additional commands, e-mail: <mailto:[EMAIL PROTECTED]> -- To unsubscribe, e-mail: <mailto:[EMAIL PROTECTED]> For additional commands, e-mail: <mailto:[EMAIL PROTECTED]>
