You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by ho...@apache.org on 2001/01/24 21:26:39 UTC
cvs commit: jakarta-tomcat-4.0/jasper/src/share/org/apache/jasper/compiler JspParseEventListener.java
horwat 01/01/24 12:26:39
Modified: jasper/src/share/org/apache/jasper/compiler
JspParseEventListener.java
Log:
Fix _jspx_init() thread safety
BR 157
Revision Changes Path
1.21 +11 -3 jakarta-tomcat-4.0/jasper/src/share/org/apache/jasper/compiler/JspParseEventListener.java
Index: JspParseEventListener.java
===================================================================
RCS file: /home/cvs/jakarta-tomcat-4.0/jasper/src/share/org/apache/jasper/compiler/JspParseEventListener.java,v
retrieving revision 1.20
retrieving revision 1.21
diff -u -r1.20 -r1.21
--- JspParseEventListener.java 2000/12/22 18:37:39 1.20
+++ JspParseEventListener.java 2001/01/24 20:26:39 1.21
@@ -1,7 +1,7 @@
/*
- * $Header: /home/cvs/jakarta-tomcat-4.0/jasper/src/share/org/apache/jasper/compiler/JspParseEventListener.java,v 1.20 2000/12/22 18:37:39 pierred Exp $
- * $Revision: 1.20 $
- * $Date: 2000/12/22 18:37:39 $
+ * $Header: /home/cvs/jakarta-tomcat-4.0/jasper/src/share/org/apache/jasper/compiler/JspParseEventListener.java,v 1.21 2001/01/24 20:26:39 horwat Exp $
+ * $Revision: 1.21 $
+ * $Date: 2001/01/24 20:26:39 $
*
* ====================================================================
*
@@ -333,8 +333,16 @@
writer.println();
writer.println("if (_jspx_inited == false) {");
writer.pushIndent();
+ writer.println("synchronized (this) {");
+ writer.pushIndent();
+ writer.println("if (_jspx_inited == false) {");
+ writer.pushIndent();
writer.println("_jspx_init();");
writer.println("_jspx_inited = true;");
+ writer.popIndent();
+ writer.println("}");
+ writer.popIndent();
+ writer.println("}");
writer.popIndent();
writer.println("}");
Re: Thread-safety
Posted by PSA <po...@posom.com>.
Pier Fumagalli wrote:
>
> Luc Vanlerberghe <lv...@bvdep.com> wrote:
>
> > Thanks for incorporating this change to jasper. I had suggested it a
> > couple of months ago (22/11/2000 in fact: see
> > http://w6.metronet.com/~wjm/tomcat/2000/Nov/msg00747.html)
> >
> > In the meantime, however, I have been browsing through the sessions of
> > the JavaOne conference of 2000 and there's (at least) one session of
> > particular interest: "Correct and Efficient Synchronization of Threads
> > in the Java Programming Environment" (The complete list including links
> > to realAudio recordings is on
> > http://java.sun.com/javaone/javaone00/replay.html)
> > Here's a direct link to the abstract:
> > http://jsp.java.sun.com/javaone/javaone2000/event.jsp?eventId=754
> >
> > One of the points that is made in that session is that even this
> > 'double-check idiom' is NOT thread-safe!
> > The exact reasons for this are not so easy to understand but basically
> > what I understood is that within the synchronized block, writes to main
> > memory can occur in any order, meaning that the value of _jspx_inited
> > can be commited to main memory while some of the results of the
> > initialisation code are still in e.g. processor registers. When the
> > thread exits the synchonized block, it is required to commit all its
> > changes to main memory, but if another processor checks the value of
> > _jspx_inited *before* acquiring the lock, it may see _jspx_inited being
> > true while not all other initialisation data has actually been written
> > to main memory.
> > For more details, please follow the links above...
>
> GOTCHA! I was looking at your post with Justy this afternoon and didn't
> understand why it's not "threadsafe"... What she committed (without the ugly
> writer.println() stuff is:
>
> if (_jspx_inited == false) {
> synchronized (this) {
> if (_jspx_inited == false) {
> _jspx_init();
> _jspx_inited = true;
> }
> }
> }
>
> So, if the commit of the _jspx_inited value can occour while _jspx_init() is
> still in progress (let's imagine some weird code optimization techniques),
> to fix this bug, we should simply remove the first line of the commit and
> simply write:
>
> synchronized (this) {
> if (_jspx_inited == false) {
> _jspx_init();
> _jspx_inited = true;
> }
> }
>
> So that, no matter what happens, a thread must ALWAYS acquire a lock on the
> synchronized piece of code, and so we are guaranteed that _jspx_init() is
> called at least once all the time...
If even this is thread-safe, you have now required that all requests
block and aquire a lock at this point. The idea of the removed test was
to prevent this necessity after the first access.
As Luc asked, why can't this code be optimized into init()?
-Paul
Re: Thread-safety
Posted by Luc Vanlerberghe <lv...@bvdep.com>.
> Does this mean that the following code would be thread safe?
NO, it's not!
Check the JavaOne session I mentioned and follow the links they gave.
There have been various patches suggested, but most of them are wrong...
The only safe way to do it is synchronize before the first test...
The reason your construct might fail is that the JLS does not allow
statements to be moved out of a synchronized block, but it is allowed to
move statements into such a block (with certain restrictions)...
So the _jspx_inited = true can be legally moved into the second
synchronized block and then moved around within it again according to
what the compiler/memory-subsysytem thinks best to optimize for speed.
It may work on 99% of all systems/compiler combinations, but Murphy's
law says that chances are still 50/50 it will fail once in a while on
your system while the boss is watching :)
These optimizations were explicitly allowed in the JL/JVM spec to allow
for speed optimizations in the compilers/JVMs/multiprocessing hardware.
Again, for more info, follow the links from the session for more info...
http://w6.metronet.com/~wjm/tomcat/2000/Nov/msg00747.html)
http://java.sun.com/javaone/javaone00/replay.html)
http://jsp.java.sun.com/javaone/javaone2000/event.jsp?eventId=754
Luc Vanlerberghe
Ethan Wallwork wrote:
>
> "When the thread exits the synchonized block, it is required to commit all its
> changes to main memory"
>
> Does this mean that the following code would be thread safe?
>
> if (_jspx_inited == false) {
> synchronized (this) {
> if (_jspx_inited == false) {
> synchronized(new Object()) {_jspx_init();}
> _jspx_inited = true;
> }
> }
> }
>
> The inner synchronized block should ensure that the initialization gets
> commited before _jpsx_inited gets set to true.
>
> Fun stuff!
>
> --
> Ethan
>
> -----Original Message-----
> From: Pier Fumagalli [mailto:pier@betaversion.org]
> Sent: Friday, January 26, 2001 5:03 AM
> To: tomcat-dev@jakarta.apache.org
> Cc: Justyna Horwat
> Subject: Re: Thread-safety
>
> Luc Vanlerberghe <lv...@bvdep.com> wrote:
>
> > Thanks for incorporating this change to jasper. I had suggested it a
> > couple of months ago (22/11/2000 in fact: see
> > http://w6.metronet.com/~wjm/tomcat/2000/Nov/msg00747.html)
> >
> > In the meantime, however, I have been browsing through the sessions of
> > the JavaOne conference of 2000 and there's (at least) one session of
> > particular interest: "Correct and Efficient Synchronization of Threads
> > in the Java Programming Environment" (The complete list including links
> > to realAudio recordings is on
> > http://java.sun.com/javaone/javaone00/replay.html)
> > Here's a direct link to the abstract:
> > http://jsp.java.sun.com/javaone/javaone2000/event.jsp?eventId=754
> >
> > One of the points that is made in that session is that even this
> > 'double-check idiom' is NOT thread-safe!
> > The exact reasons for this are not so easy to understand but basically
> > what I understood is that within the synchronized block, writes to main
> > memory can occur in any order, meaning that the value of _jspx_inited
> > can be commited to main memory while some of the results of the
> > initialisation code are still in e.g. processor registers. When the
> > thread exits the synchonized block, it is required to commit all its
> > changes to main memory, but if another processor checks the value of
> > _jspx_inited *before* acquiring the lock, it may see _jspx_inited being
> > true while not all other initialisation data has actually been written
> > to main memory.
> > For more details, please follow the links above...
>
> GOTCHA! I was looking at your post with Justy this afternoon and didn't
> understand why it's not "threadsafe"... What she committed (without the ugly
> writer.println() stuff is:
>
> if (_jspx_inited == false) {
> synchronized (this) {
> if (_jspx_inited == false) {
> _jspx_init();
> _jspx_inited = true;
> }
> }
> }
>
> So, if the commit of the _jspx_inited value can occour while _jspx_init() is
> still in progress (let's imagine some weird code optimization techniques),
> to fix this bug, we should simply remove the first line of the commit and
> simply write:
>
> synchronized (this) {
> if (_jspx_inited == false) {
> _jspx_init();
> _jspx_inited = true;
> }
> }
>
> So that, no matter what happens, a thread must ALWAYS acquire a lock on the
> synchronized piece of code, and so we are guaranteed that _jspx_init() is
> called at least once all the time...
>
> Yep, makes sense...
>
> Pier
>
> --
> Pier Fumagalli <ma...@betaversion.org>
> I'm selling my Sony Vaio Z505. Check out <http://www.betaversion.org/~pier/>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: tomcat-dev-unsubscribe@jakarta.apache.org
> For additional commands, email: tomcat-dev-help@jakarta.apache.org
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: tomcat-dev-unsubscribe@jakarta.apache.org
> For additional commands, email: tomcat-dev-help@jakarta.apache.org
RE: Thread-safety
Posted by Ethan Wallwork <et...@net-linx.com>.
"When the thread exits the synchonized block, it is required to commit all its
changes to main memory"
Does this mean that the following code would be thread safe?
if (_jspx_inited == false) {
synchronized (this) {
if (_jspx_inited == false) {
synchronized(new Object()) {_jspx_init();}
_jspx_inited = true;
}
}
}
The inner synchronized block should ensure that the initialization gets
commited before _jpsx_inited gets set to true.
Fun stuff!
--
Ethan
-----Original Message-----
From: Pier Fumagalli [mailto:pier@betaversion.org]
Sent: Friday, January 26, 2001 5:03 AM
To: tomcat-dev@jakarta.apache.org
Cc: Justyna Horwat
Subject: Re: Thread-safety
Luc Vanlerberghe <lv...@bvdep.com> wrote:
> Thanks for incorporating this change to jasper. I had suggested it a
> couple of months ago (22/11/2000 in fact: see
> http://w6.metronet.com/~wjm/tomcat/2000/Nov/msg00747.html)
>
> In the meantime, however, I have been browsing through the sessions of
> the JavaOne conference of 2000 and there's (at least) one session of
> particular interest: "Correct and Efficient Synchronization of Threads
> in the Java Programming Environment" (The complete list including links
> to realAudio recordings is on
> http://java.sun.com/javaone/javaone00/replay.html)
> Here's a direct link to the abstract:
> http://jsp.java.sun.com/javaone/javaone2000/event.jsp?eventId=754
>
> One of the points that is made in that session is that even this
> 'double-check idiom' is NOT thread-safe!
> The exact reasons for this are not so easy to understand but basically
> what I understood is that within the synchronized block, writes to main
> memory can occur in any order, meaning that the value of _jspx_inited
> can be commited to main memory while some of the results of the
> initialisation code are still in e.g. processor registers. When the
> thread exits the synchonized block, it is required to commit all its
> changes to main memory, but if another processor checks the value of
> _jspx_inited *before* acquiring the lock, it may see _jspx_inited being
> true while not all other initialisation data has actually been written
> to main memory.
> For more details, please follow the links above...
GOTCHA! I was looking at your post with Justy this afternoon and didn't
understand why it's not "threadsafe"... What she committed (without the ugly
writer.println() stuff is:
if (_jspx_inited == false) {
synchronized (this) {
if (_jspx_inited == false) {
_jspx_init();
_jspx_inited = true;
}
}
}
So, if the commit of the _jspx_inited value can occour while _jspx_init() is
still in progress (let's imagine some weird code optimization techniques),
to fix this bug, we should simply remove the first line of the commit and
simply write:
synchronized (this) {
if (_jspx_inited == false) {
_jspx_init();
_jspx_inited = true;
}
}
So that, no matter what happens, a thread must ALWAYS acquire a lock on the
synchronized piece of code, and so we are guaranteed that _jspx_init() is
called at least once all the time...
Yep, makes sense...
Pier
--
Pier Fumagalli <ma...@betaversion.org>
I'm selling my Sony Vaio Z505. Check out <http://www.betaversion.org/~pier/>
---------------------------------------------------------------------
To unsubscribe, e-mail: tomcat-dev-unsubscribe@jakarta.apache.org
For additional commands, email: tomcat-dev-help@jakarta.apache.org
Re: Thread-safety
Posted by Paul Speed <Pa...@metrixpoint.com>.
Marc Saegesser wrote:
>
> OK, if _jspx_init() can be inlined and the _jspx_inited=true assignment
> might get interspersed within the inlined code (the fog is lifting now) then
> the second approach I presented where the result of _jspx_init() is used
> should work.
Not really... for the same reason that a constructor has
problems. The code that returns the true value would get inlined as
a variable assignment... and could possibly be moved somewhere higher
based on dependencies.
I wonder how likely it is that _jspx_init() will be inlined.
-Paul
>
> > -----Original Message-----
> > From: Paul Speed [mailto:Paul.Speed@metrixpoint.com]
> > Sent: Friday, January 26, 2001 2:12 PM
> > To: tomcat-dev@jakarta.apache.org
> > Subject: Re: Thread-safety
> >
> >
> >
> >
> > Marc Saegesser wrote:
> > >
> > > This is a truly fascinating thread of discussion. However,
> > from reading the
> > > article _The "Double-Checked Locking is Broken" Declaration_
> > > (http://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html)
> > >
> > > It seems to me that the following code is thread safe.
> > >
> > > if (_jspx_inited == false) {
> > > synchronized (this) {
> > > if (_jspx_inited == false) {
> > > _jspx_init();
> > > _jspx_inited = true;
> > > }
> > > }
> > > }
> > >
> > > The case described in the article is the following
> > >
> > > class Foo {
> > > private Helper helper = null;
> > > public Helper getHelper() {
> > > if (helper == null)
> > > synchronized(this) {
> > > if (helper == null)
> > > helper = new Helper();
> > > }
> > > return helper;
> > > }
> > > // other functions and members...
> > > }
> > > }
> > >
> > > The problem is that helper may be assigned a value before the Helper
> > > constructor executes. Thus another thread may come along and notice a
> > > non-null value for helper and attempt to use un-initialized values.
> > >
> > > In the _jspx_inited case above the only requirement is that
> > compiler can't
> > > rewrite the code into the equivalent of
> > >
> > > if (_jspx_inited == false) {
> > > synchronized (this) {
> > > if (_jspx_inited == false) {
> > > _jspx_inited = true;
> > > _jspx_init();
> > > }
> > > }
> > > }
> > >
> > > Such a re-ordering seems illegal to me (what if jspx_init()
> > uses the value
> > > of _jspx_inited?). If, however, it is legal reordering then
> > the example of
> > > a "correct" double-check mechanism for 32-bit primitive values
> > should work
> > > here. It would look something like
> > >
> > > boolean tmpInited = _jspx_inited;
> > > if (tmpInited == false) {
> > > synchronized (this) {
> > > if (tmpInited == false) {
> > > tmpInited = _jspx_init(); // NOTE: _jspx_init() needs to
> > > return true
> > > _jspx_inited = tmpInited;
> > > }
> > > }
> > > }
> >
> > The issue as I understand it is that the constructor might
> > have been inlined and then the inlined instructions may have been
> > re-ordered. If _jspx_init() can be inlined then it might exhibit
> > the same problem. My question is what the spec says about inlining
> > virtual methods... I'm pretty sure that _jspx_init() is only a
> > candidate for inlining through runtime compilation by Hotspot. But
> > I'm not even sure of that.
> >
> > Otherwise, if it can never be inlined then your original
> > assertion is correct.
> > -Paul Speed
> >
> > >
> > > ---------------------------------------------------------------------
> > > To unsubscribe, e-mail: tomcat-dev-unsubscribe@jakarta.apache.org
> > > For additional commands, email: tomcat-dev-help@jakarta.apache.org
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: tomcat-dev-unsubscribe@jakarta.apache.org
> > For additional commands, email: tomcat-dev-help@jakarta.apache.org
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: tomcat-dev-unsubscribe@jakarta.apache.org
> For additional commands, email: tomcat-dev-help@jakarta.apache.org
RE: Thread-safety
Posted by Marc Saegesser <ma...@apropos.com>.
OK, if _jspx_init() can be inlined and the _jspx_inited=true assignment
might get interspersed within the inlined code (the fog is lifting now) then
the second approach I presented where the result of _jspx_init() is used
should work.
> -----Original Message-----
> From: Paul Speed [mailto:Paul.Speed@metrixpoint.com]
> Sent: Friday, January 26, 2001 2:12 PM
> To: tomcat-dev@jakarta.apache.org
> Subject: Re: Thread-safety
>
>
>
>
> Marc Saegesser wrote:
> >
> > This is a truly fascinating thread of discussion. However,
> from reading the
> > article _The "Double-Checked Locking is Broken" Declaration_
> > (http://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html)
> >
> > It seems to me that the following code is thread safe.
> >
> > if (_jspx_inited == false) {
> > synchronized (this) {
> > if (_jspx_inited == false) {
> > _jspx_init();
> > _jspx_inited = true;
> > }
> > }
> > }
> >
> > The case described in the article is the following
> >
> > class Foo {
> > private Helper helper = null;
> > public Helper getHelper() {
> > if (helper == null)
> > synchronized(this) {
> > if (helper == null)
> > helper = new Helper();
> > }
> > return helper;
> > }
> > // other functions and members...
> > }
> > }
> >
> > The problem is that helper may be assigned a value before the Helper
> > constructor executes. Thus another thread may come along and notice a
> > non-null value for helper and attempt to use un-initialized values.
> >
> > In the _jspx_inited case above the only requirement is that
> compiler can't
> > rewrite the code into the equivalent of
> >
> > if (_jspx_inited == false) {
> > synchronized (this) {
> > if (_jspx_inited == false) {
> > _jspx_inited = true;
> > _jspx_init();
> > }
> > }
> > }
> >
> > Such a re-ordering seems illegal to me (what if jspx_init()
> uses the value
> > of _jspx_inited?). If, however, it is legal reordering then
> the example of
> > a "correct" double-check mechanism for 32-bit primitive values
> should work
> > here. It would look something like
> >
> > boolean tmpInited = _jspx_inited;
> > if (tmpInited == false) {
> > synchronized (this) {
> > if (tmpInited == false) {
> > tmpInited = _jspx_init(); // NOTE: _jspx_init() needs to
> > return true
> > _jspx_inited = tmpInited;
> > }
> > }
> > }
>
> The issue as I understand it is that the constructor might
> have been inlined and then the inlined instructions may have been
> re-ordered. If _jspx_init() can be inlined then it might exhibit
> the same problem. My question is what the spec says about inlining
> virtual methods... I'm pretty sure that _jspx_init() is only a
> candidate for inlining through runtime compilation by Hotspot. But
> I'm not even sure of that.
>
> Otherwise, if it can never be inlined then your original
> assertion is correct.
> -Paul Speed
>
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: tomcat-dev-unsubscribe@jakarta.apache.org
> > For additional commands, email: tomcat-dev-help@jakarta.apache.org
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: tomcat-dev-unsubscribe@jakarta.apache.org
> For additional commands, email: tomcat-dev-help@jakarta.apache.org
Re: Thread-safety
Posted by Paul Speed <Pa...@metrixpoint.com>.
Marc Saegesser wrote:
>
> This is a truly fascinating thread of discussion. However, from reading the
> article _The "Double-Checked Locking is Broken" Declaration_
> (http://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html)
>
> It seems to me that the following code is thread safe.
>
> if (_jspx_inited == false) {
> synchronized (this) {
> if (_jspx_inited == false) {
> _jspx_init();
> _jspx_inited = true;
> }
> }
> }
>
> The case described in the article is the following
>
> class Foo {
> private Helper helper = null;
> public Helper getHelper() {
> if (helper == null)
> synchronized(this) {
> if (helper == null)
> helper = new Helper();
> }
> return helper;
> }
> // other functions and members...
> }
> }
>
> The problem is that helper may be assigned a value before the Helper
> constructor executes. Thus another thread may come along and notice a
> non-null value for helper and attempt to use un-initialized values.
>
> In the _jspx_inited case above the only requirement is that compiler can't
> rewrite the code into the equivalent of
>
> if (_jspx_inited == false) {
> synchronized (this) {
> if (_jspx_inited == false) {
> _jspx_inited = true;
> _jspx_init();
> }
> }
> }
>
> Such a re-ordering seems illegal to me (what if jspx_init() uses the value
> of _jspx_inited?). If, however, it is legal reordering then the example of
> a "correct" double-check mechanism for 32-bit primitive values should work
> here. It would look something like
>
> boolean tmpInited = _jspx_inited;
> if (tmpInited == false) {
> synchronized (this) {
> if (tmpInited == false) {
> tmpInited = _jspx_init(); // NOTE: _jspx_init() needs to
> return true
> _jspx_inited = tmpInited;
> }
> }
> }
The issue as I understand it is that the constructor might
have been inlined and then the inlined instructions may have been
re-ordered. If _jspx_init() can be inlined then it might exhibit
the same problem. My question is what the spec says about inlining
virtual methods... I'm pretty sure that _jspx_init() is only a
candidate for inlining through runtime compilation by Hotspot. But
I'm not even sure of that.
Otherwise, if it can never be inlined then your original
assertion is correct.
-Paul Speed
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: tomcat-dev-unsubscribe@jakarta.apache.org
> For additional commands, email: tomcat-dev-help@jakarta.apache.org
Re: FW: Thread-safety
Posted by Brian Goetz <br...@quiotix.com>.
>This is a truly fascinating thread of discussion. However, from reading the
>article _The "Double-Checked Locking is Broken" Declaration_
>(http://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html)
>
>It seems to me that the following code is thread safe.
>
>if (_jspx_inited == false) {
> synchronized (this) {
> if (_jspx_inited == false) {
> _jspx_init();
> _jspx_inited = true;
> }
> }
>}
This depends on a lot of things. In particular, it depends on whether
_jspx_init() creates any objects as a side-effect. If it does, then this
is just another broken attempt to fix the double-checked lock
problem. (The memory-operation reordering thing is just more complicated
than it looks.)
Not only can a compiler reorder instructions within a synchronized block
(and may also move instructions from outside the block into the block, and
then reorder), but the memory system can make it appear that instructions
are executed out of order unless BOTH threads synchronize on the SAME lock.
If _jspx_init() were to create an object and assign a reference to it
somewhere, then you could still obtain a reference to a partially
constructed object just like with double-checked-locking.
Assuming jspx_init() might create an object (what else would an init()
routine do?), the problem is that your initial reference of jspx_inited is
unsynchronized. Supposed thread A is inside the synchronized
block. Thread B is about to execute "if (_jspx_inited == false). The
compiler/cache/memory CAN make it appear to thread B that jspx_inited has
been set to true before all the instructions corresponding to jspx_init()
have executed.
>In the _jspx_inited case above the only requirement is that compiler can't
>rewrite the code into the equivalent of
>
> _jspx_inited = true;
> _jspx_init();
>
>Such a re-ordering seems illegal to me (what if jspx_init() uses the value
>of _jspx_inited?).
It might seem illegal, but it isn't. They could be executed in the
expected order on one processor, but the memory write corresponding to
jspx_inited might become visible to thread B before the memory writes
generated by jspx_init(), unless the reference to jspx_inited is inside a
synchronized, which it isn't.
But even ignoring memory-system reordering (which you can't) your intuition
is still incorrect. The init() can be inlined, and instructions can be
arbitrarily reordered as long as the compiler/JVM knows that it is not
violating dataflow assumptions (one instruction requires the result of
another) and that the intervening instructions won't throw an exception.
>If, however, it is legal reordering then the example of
>a "correct" double-check mechanism for 32-bit primitive values should work
>here. It would look something like
>
>boolean tmpInited = _jspx_inited;
>if (tmpInited == false) {
> synchronized (this) {
> if (tmpInited == false) {
> tmpInited = _jspx_init(); // NOTE: _jspx_init() needs to
>return true
> _jspx_inited = tmpInited;
> }
> }
>}
Still doesn't fix the problem. jspx_init() can be inlined. Since the
return value (true) doesn't depend on other things done by jspx_init(),
jspx_inited can be set to true (or appear to be so from another thread)
before all the operations of jspx_init() are finished.
What if jspx_init() is defined like this:
public boolean jspx_init() {
foo = InitSomething();
return true;
}
You've still not created a strong enough dataflow requirement to force the
compiler to initialize foo before assigning jspx_inited. And even if you
had, there'd still be the problem of the appearance of reordering because
of caching. The only cure for that is synchronization.
If jspx_init() creates objects or sets any externally accessible value
other than its return value, then you can't make an end-run around the DCL
problem this way.
--
Brian Goetz
Quiotix Corporation
brian@quiotix.com Tel: 650-843-1300 Fax: 650-324-8032
http://www.quiotix.com
RE: Thread-safety
Posted by Marc Saegesser <ma...@apropos.com>.
This is a truly fascinating thread of discussion. However, from reading the
article _The "Double-Checked Locking is Broken" Declaration_
(http://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html)
It seems to me that the following code is thread safe.
if (_jspx_inited == false) {
synchronized (this) {
if (_jspx_inited == false) {
_jspx_init();
_jspx_inited = true;
}
}
}
The case described in the article is the following
class Foo {
private Helper helper = null;
public Helper getHelper() {
if (helper == null)
synchronized(this) {
if (helper == null)
helper = new Helper();
}
return helper;
}
// other functions and members...
}
}
The problem is that helper may be assigned a value before the Helper
constructor executes. Thus another thread may come along and notice a
non-null value for helper and attempt to use un-initialized values.
In the _jspx_inited case above the only requirement is that compiler can't
rewrite the code into the equivalent of
if (_jspx_inited == false) {
synchronized (this) {
if (_jspx_inited == false) {
_jspx_inited = true;
_jspx_init();
}
}
}
Such a re-ordering seems illegal to me (what if jspx_init() uses the value
of _jspx_inited?). If, however, it is legal reordering then the example of
a "correct" double-check mechanism for 32-bit primitive values should work
here. It would look something like
boolean tmpInited = _jspx_inited;
if (tmpInited == false) {
synchronized (this) {
if (tmpInited == false) {
tmpInited = _jspx_init(); // NOTE: _jspx_init() needs to
return true
_jspx_inited = tmpInited;
}
}
}
Re: Thread-safety
Posted by Pier Fumagalli <pi...@betaversion.org>.
Luc Vanlerberghe <lv...@bvdep.com> wrote:
> Thanks for incorporating this change to jasper. I had suggested it a
> couple of months ago (22/11/2000 in fact: see
> http://w6.metronet.com/~wjm/tomcat/2000/Nov/msg00747.html)
>
> In the meantime, however, I have been browsing through the sessions of
> the JavaOne conference of 2000 and there's (at least) one session of
> particular interest: "Correct and Efficient Synchronization of Threads
> in the Java Programming Environment" (The complete list including links
> to realAudio recordings is on
> http://java.sun.com/javaone/javaone00/replay.html)
> Here's a direct link to the abstract:
> http://jsp.java.sun.com/javaone/javaone2000/event.jsp?eventId=754
>
> One of the points that is made in that session is that even this
> 'double-check idiom' is NOT thread-safe!
> The exact reasons for this are not so easy to understand but basically
> what I understood is that within the synchronized block, writes to main
> memory can occur in any order, meaning that the value of _jspx_inited
> can be commited to main memory while some of the results of the
> initialisation code are still in e.g. processor registers. When the
> thread exits the synchonized block, it is required to commit all its
> changes to main memory, but if another processor checks the value of
> _jspx_inited *before* acquiring the lock, it may see _jspx_inited being
> true while not all other initialisation data has actually been written
> to main memory.
> For more details, please follow the links above...
GOTCHA! I was looking at your post with Justy this afternoon and didn't
understand why it's not "threadsafe"... What she committed (without the ugly
writer.println() stuff is:
if (_jspx_inited == false) {
synchronized (this) {
if (_jspx_inited == false) {
_jspx_init();
_jspx_inited = true;
}
}
}
So, if the commit of the _jspx_inited value can occour while _jspx_init() is
still in progress (let's imagine some weird code optimization techniques),
to fix this bug, we should simply remove the first line of the commit and
simply write:
synchronized (this) {
if (_jspx_inited == false) {
_jspx_init();
_jspx_inited = true;
}
}
So that, no matter what happens, a thread must ALWAYS acquire a lock on the
synchronized piece of code, and so we are guaranteed that _jspx_init() is
called at least once all the time...
Yep, makes sense...
Pier
--
Pier Fumagalli <ma...@betaversion.org>
I'm selling my Sony Vaio Z505. Check out <http://www.betaversion.org/~pier/>
Thread-safety
Posted by Luc Vanlerberghe <lv...@bvdep.com>.
Thanks for incorporating this change to jasper. I had suggested it a
couple of months ago (22/11/2000 in fact: see
http://w6.metronet.com/~wjm/tomcat/2000/Nov/msg00747.html)
In the meantime, however, I have been browsing through the sessions of
the JavaOne conference of 2000 and there's (at least) one session of
particular interest: "Correct and Efficient Synchronization of Threads
in the Java Programming Environment" (The complete list including links
to realAudio recordings is on
http://java.sun.com/javaone/javaone00/replay.html)
Here's a direct link to the abstract:
http://jsp.java.sun.com/javaone/javaone2000/event.jsp?eventId=754
One of the points that is made in that session is that even this
'double-check idiom' is NOT thread-safe!
The exact reasons for this are not so easy to understand but basically
what I understood is that within the synchronized block, writes to main
memory can occur in any order, meaning that the value of _jspx_inited
can be commited to main memory while some of the results of the
initialisation code are still in e.g. processor registers. When the
thread exits the synchonized block, it is required to commit all its
changes to main memory, but if another processor checks the value of
_jspx_inited *before* acquiring the lock, it may see _jspx_inited being
true while not all other initialisation data has actually been written
to main memory.
For more details, please follow the links above...
One if the questions I raised in my earlier post is why this
initialisation code isn't moved to the init() method of the generated
servlet? This moves the responsability for executing the initialization
exactly once to the container where IMHO it belongs...
Luc Vanlerberghe
horwat@apache.org wrote:
>
> horwat 01/01/24 12:26:39
>
> Modified: jasper/src/share/org/apache/jasper/compiler
> JspParseEventListener.java
> Log:
> Fix _jspx_init() thread safety
>
> BR 157
>
> Revision Changes Path
> 1.21 +11 -3 jakarta-tomcat-4.0/jasper/src/share/org/apache/jasper/compiler/JspParseEventListener.java
>
> Index: JspParseEventListener.java
> ===================================================================
> RCS file: /home/cvs/jakarta-tomcat-4.0/jasper/src/share/org/apache/jasper/compiler/JspParseEventListener.java,v
> retrieving revision 1.20
> retrieving revision 1.21
> diff -u -r1.20 -r1.21
> --- JspParseEventListener.java 2000/12/22 18:37:39 1.20
> +++ JspParseEventListener.java 2001/01/24 20:26:39 1.21
> @@ -1,7 +1,7 @@
> /*
> - * $Header: /home/cvs/jakarta-tomcat-4.0/jasper/src/share/org/apache/jasper/compiler/JspParseEventListener.java,v 1.20 2000/12/22 18:37:39 pierred Exp $
> - * $Revision: 1.20 $
> - * $Date: 2000/12/22 18:37:39 $
> + * $Header: /home/cvs/jakarta-tomcat-4.0/jasper/src/share/org/apache/jasper/compiler/JspParseEventListener.java,v 1.21 2001/01/24 20:26:39 horwat Exp $
> + * $Revision: 1.21 $
> + * $Date: 2001/01/24 20:26:39 $
> *
> * ====================================================================
> *
> @@ -333,8 +333,16 @@
> writer.println();
> writer.println("if (_jspx_inited == false) {");
> writer.pushIndent();
> + writer.println("synchronized (this) {");
> + writer.pushIndent();
> + writer.println("if (_jspx_inited == false) {");
> + writer.pushIndent();
> writer.println("_jspx_init();");
> writer.println("_jspx_inited = true;");
> + writer.popIndent();
> + writer.println("}");
> + writer.popIndent();
> + writer.println("}");
> writer.popIndent();
> writer.println("}");
>
>
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: tomcat-dev-unsubscribe@jakarta.apache.org
> For additional commands, email: tomcat-dev-help@jakarta.apache.org