You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@struts.apache.org by bu...@apache.org on 2002/04/16 02:28:35 UTC
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: struts-dev@jakarta.apache.org
ReportedBy: chuckcavaness@attbi.com
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: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
RE: The synchronized keyword is used too early in the
processActionCreate() method of the RequestProcessor class
Posted by Chuck Cavaness <ch...@attbi.com>.
Donnie,
Very good explantation. I get it now. Thanks for taking the time.
Chuck
At 10:07 PM 4/15/2002 -0400, you wrote:
>See below.
>
> >
> > 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.
>
>First, I mean atomic here in the sense that they all have to occur within a
>sychronized block, not in the database sense of atomicity, succeed-fail etc.
>
>Second, I'll try to more carefully explain the race condition that can occur
>if the synchronization doesn't begin before checking the map. I'll use your
>scenario, which in pseudocode is:
>
>1) Action actn = map.get(actionType);
>2) if (actn == null) {
>3) synchronized {
>4) Action actn = new actionType;
>5) map.put(actionType, actn);
>6) }
> }
> return actn;
>
>We have two threads, "a" and "b" arriving at this code sequence, racing to
>get through; and both threads are interested in the same action type.
>
>Thread "a" executes 1) and 2); then a context switch occurs. At that point,
>there is still not an Action of the appropriate type in the map.
>
>So thread "b" executes 1), 2), and 3); it's got the lock now.
>
>Thread "a" wakes up and tries to do 3), but can't so it blocks.
>
>Thread "b" wakes up again. It executes 4), 5), and 6); now there's an Action
>of the appropriate type in the map, and thread "b" goes merrily on its way.
>
>Thread "a" wakes back up and can now obtain the lock. It executes 3), 4),
>5), and 6). So a second Action of the same action type has been put int the
>map; since HashMap only allows one for a given key, it overwrites the first
>Action that thread "b" had put in there.
>
>Again, in this narrow scenario, it may be acceptable to live with that since
>getting a second one that's functionally equivalent to the first, and
>letting the first get gc'd, isn't a big deal. Another option would be to
>make the map functionally read-only by creating all the Actions at
>initialization time rather than "just-in-time". Not sure of the implications
>of that idea, though.
>
>This precise scenario is one reason why the whole concept of "thread-safe"
>containers like the original HashTable are a bad idea in my opinion. Sure,
>it may make access to the internal data structures safe for the duration of
>any single method call on the object; but it doesn't help any with the
>logical scenarios via which a container is accessed.
>
> >
> > 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.
>
>I only mentioned the double-check pattern because someone might say that
>that would be a valid optimization, and I wanted to preemptively respond to
>that. :)
>
>Hope that all helps,
>
>Donnie
>
>
>--
>To unsubscribe, e-mail: <ma...@jakarta.apache.org>
>For additional commands, e-mail: <ma...@jakarta.apache.org>
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
RE: The synchronized keyword is used too early in the processActionCreate() method of the RequestProcessor class
Posted by Donnie Hale <do...@haleonline.net>.
See below.
>
> 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.
First, I mean atomic here in the sense that they all have to occur within a
sychronized block, not in the database sense of atomicity, succeed-fail etc.
Second, I'll try to more carefully explain the race condition that can occur
if the synchronization doesn't begin before checking the map. I'll use your
scenario, which in pseudocode is:
1) Action actn = map.get(actionType);
2) if (actn == null) {
3) synchronized {
4) Action actn = new actionType;
5) map.put(actionType, actn);
6) }
}
return actn;
We have two threads, "a" and "b" arriving at this code sequence, racing to
get through; and both threads are interested in the same action type.
Thread "a" executes 1) and 2); then a context switch occurs. At that point,
there is still not an Action of the appropriate type in the map.
So thread "b" executes 1), 2), and 3); it's got the lock now.
Thread "a" wakes up and tries to do 3), but can't so it blocks.
Thread "b" wakes up again. It executes 4), 5), and 6); now there's an Action
of the appropriate type in the map, and thread "b" goes merrily on its way.
Thread "a" wakes back up and can now obtain the lock. It executes 3), 4),
5), and 6). So a second Action of the same action type has been put int the
map; since HashMap only allows one for a given key, it overwrites the first
Action that thread "b" had put in there.
Again, in this narrow scenario, it may be acceptable to live with that since
getting a second one that's functionally equivalent to the first, and
letting the first get gc'd, isn't a big deal. Another option would be to
make the map functionally read-only by creating all the Actions at
initialization time rather than "just-in-time". Not sure of the implications
of that idea, though.
This precise scenario is one reason why the whole concept of "thread-safe"
containers like the original HashTable are a bad idea in my opinion. Sure,
it may make access to the internal data structures safe for the duration of
any single method call on the object; but it doesn't help any with the
logical scenarios via which a container is accessed.
>
> 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.
I only mentioned the double-check pattern because someone might say that
that would be a valid optimization, and I wanted to preemptively respond to
that. :)
Hope that all helps,
Donnie
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
RE: DO NOT REPLY [Bug 8138] New: - The synchronized
keyword is used too early in the processActionCreate() method of the
RequestProcessor class
Posted by Chuck Cavaness <ch...@attbi.com>.
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: bugzilla@apache.org [mailto:bugzilla@apache.org]
> > Sent: Monday, April 15, 2002 8:29 PM
> > To: struts-dev@jakarta.apache.org
> > 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: struts-dev@jakarta.apache.org
> > ReportedBy: chuckcavaness@attbi.com
> >
> >
> > 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:
><ma...@jakarta.apache.org>
>For additional commands, e-mail: <ma...@jakarta.apache.org>
>
>
>
>--
>To unsubscribe, e-mail: <ma...@jakarta.apache.org>
>For additional commands, e-mail: <ma...@jakarta.apache.org>
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
RE: DO NOT REPLY [Bug 8138] New: - The synchronized keyword is used too early in the processActionCreate() method of the RequestProcessor class
Posted by Donnie Hale <do...@haleonline.net>.
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: bugzilla@apache.org [mailto:bugzilla@apache.org]
> Sent: Monday, April 15, 2002 8:29 PM
> To: struts-dev@jakarta.apache.org
> 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: struts-dev@jakarta.apache.org
> ReportedBy: chuckcavaness@attbi.com
>
>
> 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:
<ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>