Donnie,
Thanks for the info, but a couple things puzzle me and if you don't mind, I
would like to test some of what you said. I want to get this right in the
book, so if you and the rest of the dev list will indulge me for a couple
of emails.
You mentioned that the three steps are atomic. To me, atomic means that
they all succeed or the operation set fails. I don't think this is the case
here. If the lookup finds an action, the other operations are not executed.
To me, the atomic part is, if the action is not found in the map:
1) Start Atomic operation.
2) Create an instance of the Action.
3) Store the instance in the Map.
4) Stop atomic operation.
I believe this is the set of operations that should be atomic. By ensuring
that these operations succeed without interruption, is the main goal.
As far as double-check pattern goes, I don't believe moving synchronized
down just to around the action create changes anything with respect to
double-check. As far as I understand it, the double-check problem has to do
with certain optimizing compilers having the freedom to rearrange the
creation during a constructor call, and the actual setting of the field
reference of the new object that was just created in the constructor. If
one thread calls the constructor and an object is created, another thread
may check the field reference and not see it because the compiler has
rearranged the execution.
Moving the synchronized underneath the get call to the HashMap doesn't
introduce any more risk that's already present. Only one thread is going to
be allowed into the applicationInstance() method, no?
Chuck
P.s Just trying to understand more.
At 09:03 PM 4/15/2002 -0400, you wrote:
>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]>
--
To unsubscribe, e-mail: <mailto:[EMAIL PROTECTED]>
For additional commands, e-mail: <mailto:[EMAIL PROTECTED]>
