commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From sebb <seb...@gmail.com>
Subject Re: [VOTE] Release Commons Chain 1.2 based on RC2
Date Sat, 24 May 2008 12:36:54 GMT
On 24/05/2008, Niall Pemberton <niall.pemberton@gmail.com> wrote:
> On Sat, May 24, 2008 at 2:13 AM, sebb <sebbaz@gmail.com> wrote:
>  > On 24/05/2008, Niall Pemberton <niall.pemberton@gmail.com> wrote:
>  >> On Fri, May 23, 2008 at 11:56 PM, sebb <sebbaz@gmail.com> wrote:
>  >>  > On 23/05/2008, Niall Pemberton <niall.pemberton@gmail.com> wrote:
>  >>  >> On Fri, May 23, 2008 at 7:17 PM, sebb <sebbaz@gmail.com> wrote:
>  >>  >>  > On 23/05/2008, Niall Pemberton <niall.pemberton@gmail.com>
wrote:
>  >>  >>  >> On Fri, May 23, 2008 at 4:51 PM, sebb <sebbaz@gmail.com>
wrote:
>  >>  >>  >>  > On 23/05/2008, Niall Pemberton <niall.pemberton@gmail.com>
wrote:
>  >>  >>  >>  >> On Fri, May 23, 2008 at 3:31 PM, Luc Maisonobe
<Luc.Maisonobe@free.fr> wrote:
>  >>  >>  >>  >>  > Niall Pemberton a écrit :
>  >>  >>  >>  >>  >> On Thu, May 22, 2008 at 9:35 PM, Luc
Maisonobe <Luc.Maisonobe@free.fr> wrote:
>  >>  >>  >>  >>  >>> A few comments on this release.
>  >>  >>  >>  >>  >>>
>  >>  >>  >>  >>  >>> Typo in the project description
in the pom.xml file: replace
>  >>  >>  >>  >>  >>> "implmentation" with "implementation".
>  >>  >>  >>  >>  >>>
>  >>  >>  >>  >>  >>> Extracting files from the commons-chain-1.2-src.tar.gz
archive in a
>  >>  >>  >>  >>  >>> Linux box leads to an all lower
case file name for "license-header.txt",
>  >>  >>  >>  >>  >>> which leads to an error when running
"mvn site". Some plugin requires a
>  >>  >>  >>  >>  >>> mixed case LICENSE-header.txt.
>  >>  >>  >>  >>  >>
>  >>  >>  >>  >>  >> Thanks, I fixed the typo and checkstyle
config in the trunk:
>  >>  >>  >>  >>  >>   http://svn.apache.org/viewvc?view=rev&revision=659361
>  >>  >>  >>  >>  >>
>  >>  >>  >>  >>  >> Anyone think we need a new RC for this?
>  >>  >>  >>  >>  >
>  >>  >>  >>  >>  > No, it is really minor.
>  >>  >>  >>  >>  >
>  >>  >>  >>  >>  >>
>  >>  >>  >>  >>  >>> There are 39 findbugs errors. They
don't seem too important. Many are
>  >>  >>  >>  >>  >>> serialization related (missing
serialVersionUID, transient fields) and
>  >>  >>  >>  >>  >>> many are style related (redeclaration
of interfaces from superclass). I
>  >>  >>  >>  >>  >>> think the errors in ContextBase
and web.ChainListener are false
>  >>  >>  >>  >>  >>> positive. The MTIA_SUSPECT_SERVLET_INSTANCE_FIELD
may be more
>  >>  >>  >>  >>  >>> problematic, I know nothing about
servlets so cannot judge this. I'm
>  >>  >>  >>  >>  >>> attaching the findbug.html report
file to this message.
>  >>  >>  >>  >>  >>
>  >>  >>  >>  >>  >> I don't see it attached - also I added
findbugs to the pom and ran it
>  >>  >>  >>  >>  >> and didn't see such an error
>  >>  >>  >>  >>  >>   http://svn.apache.org/viewvc?view=rev&revision=659363
>  >>  >>  >>  >>  >
>  >>  >>  >>  >>  > Ooops. I forgot to join the page. Here
it is. It was generated by
>  >>  >>  >>  >>  > version 1.1.1 of the findbugs plugin, and
the class files were compiled
>  >>  >>  >>  >>  > with SunJDK 1.6 on a linux box.
>  >>  >>  >>  >>
>  >>  >>  >>  >>
>  >>  >>  >>  >> Maybe its getting removed by the mailing list,
because I still don't
>  >>  >>  >>  >>  see it. I tried changing the version to 1.1.1
of findbugs and used JDK
>  >>  >>  >>  >>  1.6 - but I still don't see it. AnywayI looked
up that error here:
>  >>  >>  >>  >>
>  >>  >>  >>  >>  http://findbugs.sourceforge.net/bugDescriptions.html#MTIA_SUSPECT_SERVLET_INSTANCE_FIELD
>  >>  >>  >>  >>
>  >>  >>  >>  >>  ...so I guess it must be referring to these
fields:
>  >>  >>  >>  >>  http://people.apache.org/~niallp/chain_1_2_RC2/site/xref/org/apache/commons/chain/web/servlet/ChainProcessor.html#94
>  >>  >>  >>  >>
>  >>  >>  >>  >>  In thoses cases then this warning is not an
issue - since its fine for
>  >>  >>  >>  >>  all threads to use the same instance variables
- three are just the
>  >>  >>  >>  >>  names of attributes configured for the servlet.
CatalogFactory is a
>  >>  >>  >>  >>  singleton-per-ClassLoader and one instance should
be shared by all
>  >>  >>  >>  >>  threads for the servlet instance. Looking at
the code I believe it
>  >>  >>  >>  >>  "caches" the factory in the servlet to avoid
the *synchronized* lookup
>  >>  >>  >>  >>  in CatalogFactory.getInstance():
>  >>  >>  >>  >>
>  >>  >>  >>  >
>  >>  >>  >>  > If the same instance is used by multiple threads,
then any instance
>  >>  >>  >>  > variables need to be either final, volatile or synchronized.
>  >>  >>  >>
>  >>  >>  >>
>  >>  >>  >> I don't think so in this case. These are *private* variables
that are
>  >>  >>  >>  set up by the Servlet when it is intialized[1] and removed
when it
>  >>  >>  >>  shut down[2] - these are the only times they're changed
and are not
>  >>  >>  >>  accesible by multiple threads. Once the servlet is intialialized
then
>  >>  >>  >>  they are *read* by multiple threads[3] as each request
is processed.
>  >>  >>  >>  So this is the case of *set-once* in a *thread-safe* manner
and then
>  >>  >>  >>  unchangable.
>  >>  >>  >>
>  >>  >>  >
>  >>  >>  > The JVM does not guarantee that values written to an instance
variable
>  >>  >>  > in one thread will be visible to another thread unless the
variables
>  >>  >>  > are final, volatile or protected by synch. using the *same*
lock in
>  >>  >>  > both threads. Both the writer *and reader* must synch. on the
same
>  >>  >>  > lock.
>  >>  >>
>  >>  >>
>  >>  >> This really isn't an issue. The servlet container manages the
>  >>  >>  lifecycle of these objects and doesn't allow any threads to access
the
>  >>  >>  service() method until initialization is complete. I didn't write
this
>  >>  >>  code - it was written by Craig McClanahan[1] who among other things
>  >>  >>  wrote a large part of Tomcat 4 and invented Struts 1. Struts 1's
>  >>  >>  ActionServlet did this as well - since 2000 - the most popular web
>  >>  >>  framework ever - and believe me if it had been an issue, it would
have
>  >>  >>  come up.
>  >>  >>
>  >>  >
>  >>  > There may (must?) be code in the container that performs the necessary
>  >>  > work to ensure that the memory updates are published to other threads.
>  >>
>  >>
>  >> I think you're missing the point - if these variables were being
>  >>  changed then I agree there would be a thread safety issue. But they're
>  >>  not - they being set when the container intializes the servlet - at
>  >>  that point no threads are allowed to access the service() method
>  >>  (where they are read) until intialization ia complete. After that they
>  >>  remain unchanged being accessed by multiple threads - until the
>  >>  servlet is shut down by the container.
>  >>
>  >>  Hopefully that clarifies things for you.
>  >
>  > Yes, I understand that.
>  >
>  > I'm not saying that there is an issue with multiple threads trying to
>  > update the same variables. It's not necessary to use synchronization
>  > to guarantee atomicity of updates if only a single thread is involved.
>  >
>  > However, synchronization (or final or volatile) is also needed for
>  > memory visibility.
>  >
>  > If a variable is updated in one thread only, the same thread will see
>  > the last value it wrote.
>  >
>  > However, without synch (or volatile or final), a different thread
>  > which reads the same variable will not necessarily see the last value
>  > written by the other thread.
>  >
>  > The JVM may have cached one or more values in registers for a particular thread.
>  > So the writing thread needs to be forced to update main memory, and
>  > the reading thread needs to be told to refresh its cache. This is one
>  > of the things synchronization does.
>  >
>  > Volatile variables are visible across threads because the JVM is not
>  > allowed to cache them.
>
>
> Well if it was going to be an issue, then it would have shown up in
>  struts or here in Chain. I don't have knowledge of the internals of
>  servlet containers so perhaps if you're interested then you should
>  take it up on something like the Tomcat user list. In the absence of a
>  real issue then I don't think theres any action to take and definetly
>  no issue with the release - especially since the code in that class is
>  unchanged since the initial 1.0 release back in Dec 2004.
>

Fine.

I was just pointing out that the code reported by Findbugs will only
work under certain conditions. These conditions presumably hold,
otherwise someone would likely have seen a problem (and probably
reported it).

The code does not document the assumptions needed to guarantee thread-safety.
Perhaps that should be addressed at some point.

>  Niall
>
>
>  >>  Niall
>  >>
>  >>
>  >>  > If not, then the code may work - but is not guaranteed to.
>  >>  >
>  >>  > Threads cannot share instance variables safely unless some means is
>  >>  > used to ensure memory visibility.
>  >>  >
>  >>  >>  [1] http://svn.apache.org/viewvc?view=rev&revision=142828
>  >>  >>
>  >>  >>
>  >>  >>  Niall
>  >>  >>
>  >>  >>
>  >>  >>  > I know it seems strange and counter-intuitive, but the Java
memory
>  >>  >>  > model allows threads to cache variables in registers and use
various
>  >>  >>  > other techniques for performance reasons.
>  >>  >>  >
>  >>  >>  > Without synchronization, all one can say is that the values
seen by
>  >>  >>  > the reader thread will be either the default value or the value
>  >>  >>  > written by the writer thread.
>  >>  >>  >
>  >>  >>  > Though even that may not be true:
>  >>  >>  > if there are two writer threads updating a long or double value
>  >>  >>  > without synch. it is possible that a different thread will
see a value
>  >>  >>  > made up of 32 bits set by one thread and 32 bits by another.
This
>  >>  >>  > cannot happen here because there is only one thread writing
the
>  >>  >>  > variables.
>  >>  >>  >
>  >>  >>  > For the details, I can thoroughly recommend Java Concurrency
in Practice.
>  >>  >>  >
>  >>  >>  > Also:
>  >>  >>  > http://today.java.net/pub/a/today/2004/04/13/JSR133.html
>  >>  >>  >
>  >>  >>  >>  Niall
>  >>  >>  >>
>  >>  >>  >>  [1] http://people.apache.org/~niallp/chain_1_2_RC2/site/xref/org/apache/commons/chain/web/servlet/ChainProcessor.html#145
>  >>  >>  >>  [2] http://people.apache.org/~niallp/chain_1_2_RC2/site/xref/org/apache/commons/chain/web/servlet/ChainProcessor.html#131
>  >>  >>  >>  [3] http://people.apache.org/~niallp/chain_1_2_RC2/site/xref/org/apache/commons/chain/web/servlet/ChainProcessor.html#175
>  >>  >>  >>
>  >>  >>  >>
>  >>  >>  >>  > Without one of these, there is no guarantee of memory
visibility -
>  >>  >>  >>  > thread A can set the value  of "catalog" and thread
B may never see
>  >>  >>  >>  > it.
>  >>  >>  >>  >
>  >>  >>  >>  >>  http://people.apache.org/~niallp/chain_1_2_RC2/site/xref/org/apache/commons/chain/CatalogFactory.html#178
>  >>  >>  >>  >>
>  >>  >>  >>  >>  So if I'm looking at the right place then I
don't believe this is an
>  >>  >>  >>  >>  issue - if I'm not looking in the right place
then please point me to
>  >>  >>  >>  >>  the line number(s) your findbugs report is showing
in the rc2 xref:
>  >>  >>  >>  >>  http://people.apache.org/~niallp/chain_1_2_RC2/site/xref/index.html
>  >>  >>  >>  >>
>  >>  >>  >>  >>  Thanks
>  >>  >>  >>  >>
>  >>  >>  >>  >>
>  >>  >>  >>  >>  Niall
>  >>  >>  >>  >>
>  >>  >>  >>  >>
>  >>  >>  >>  >>  > Luc
>  >>  >>  >>  >>  >>
>  >>  >>  >>  >>  >> Niall
>  >>  >>  >>  >>  >>
>  >>  >>  >>  >>  >>> I don't cast any vote now, waiting
for more knowledgeable people to look
>  >>  >>  >>  >>  >>> at these servlet issues.
>  >>  >>  >>  >>  >>>
>  >>  >>  >>  >>  >>> Luc
>  >>  >>  >>  >>  >>>
>  >>  >>  >>  >>  >>> Oliver Heger wrote:
>  >>  >>  >>  >>  >>>> +1
>  >>  >>  >>  >>  >>>>
>  >>  >>  >>  >>  >>>> Oliver
>  >>  >>  >>  >>  >>>>
>  >>  >>  >>  >>  >>>> Niall Pemberton wrote:
>  >>  >>  >>  >>  >>>>> The main changes since
RC1 are that the ant build now works on JDK 1.3
>  >>  >>  >>  >>  >>>>> and the Logging dependency
has been upgraded to the latest 1.1.1
>  >>  >>  >>  >>  >>>>>
>  >>  >>  >>  >>  >>>>> The artifacts are here:
>  >>  >>  >>  >>  >>>>> http://people.apache.org/~niallp/chain_1_2_RC2/
>  >>  >>  >>  >>  >>>>>
>  >>  >>  >>  >>  >>>>> SVN Tag:
>  >>  >>  >>  >>  >>>>> http://svn.apache.org/viewvc/commons/proper/chain/tags/CHAIN_1_2_RC2/
>  >>  >>  >>  >>  >>>>>
>  >>  >>  >>  >>  >>>>> Site:
>  >>  >>  >>  >>  >>>>> http://people.apache.org/~niallp/chain_1_2_RC2/site/
>  >>  >>  >>  >>  >>>>> (note m2 generates relative
links, so some don't work - but the site
>  >>  >>  >>  >>  >>>>> is for info and not included
in the release artifacts)
>  >>  >>  >>  >>  >>>>>
>  >>  >>  >>  >>  >>>>> Release Notes:
>  >>  >>  >>  >>  >>>>> http://people.apache.org/~niallp/chain_1_2_RC2/RELEASE-NOTES.txt
>  >>  >>  >>  >>  >>>>> http://people.apache.org/~niallp/chain_1_2_RC2/site/changes-report.html
>  >>  >>  >>  >>  >>>>>
>  >>  >>  >>  >>  >>>>> RAT Report:
>  >>  >>  >>  >>  >>>>> http://people.apache.org/~niallp/chain_1_2_RC2/site/rat-report.html
>  >>  >>  >>  >>  >>>>>
>  >>  >>  >>  >>  >>>>> CLIRR Report:
>  >>  >>  >>  >>  >>>>> http://people.apache.org/~niallp/chain_1_2_RC2/site/clirr-report.html
>  >>  >>  >>  >>  >>>>>
>  >>  >>  >>  >>  >>>>> RC2 has been built with
m2 - but m1 and ant builds are available - details here:
>  >>  >>  >>  >>  >>>>> http://people.apache.org/~niallp/chain_1_2_RC2/site/building.html
>  >>  >>  >>  >>  >>>>>
>  >>  >>  >>  >>  >>>>> Note: Chain is targetted
at JDK 1.3, but I built with JDK 1.5 because
>  >>  >>  >>  >>  >>>>> of the issue with m2 and
JDK 1.4 - but I have tested on JDK 1.3 and
>  >>  >>  >>  >>  >>>>> JDK 1.4 using m1 &
ant and JDK 1.5 and JDK 1.6 using m2
>  >>  >>  >>  >>  >>>>>
>  >>  >>  >>  >>  >>>>> Vote is open for 72 hours
>  >>  >>  >>  >>  >>>>>
>  >>  >>  >>  >>  >>>>> Thanks in advance for your
feedback/votes.
>  >>  >>  >>  >>  >>>>>
>  >>  >>  >>  >>  >>>>> Niall
>  >>  >>  >>  >>  >>>>> ------------------------------------------------------------------------------------------------------------->
>  >>  >>  >>  >>  >>>>>
>  >>  >>  >>  >>  >>>>> [  ] +1  I support this
release
>  >>  >>  >>  >>  >>>>> [  ] +0  I am OK with this
release
>  >>  >>  >>  >>  >>>>> [  ] -0   OK, but....
>  >>  >>  >>  >>  >>>>> [  ] -1   I do not support
this release
>  >>  >>  >>  >>  >>>>>
>  >>  >>  >>  >>  >>>>> ---------------------------------------------------------------------
>  >>  >>  >>  >>
>  >>  >>  >>
>  >>  >>  >>  ---------------------------------------------------------------------
>  >>  >>  >>  To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>  >>  >>  >>  For additional commands, e-mail: dev-help@commons.apache.org
>  >>  >>  >>
>  >>  >>  >>
>  >>  >>  >
>  >>  >>  > ---------------------------------------------------------------------
>  >>  >>  > To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>  >>  >>  > For additional commands, e-mail: dev-help@commons.apache.org
>  >>  >>  >
>  >>  >>  >
>  >>  >>
>  >>  >>  ---------------------------------------------------------------------
>  >>  >>  To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>  >>  >>  For additional commands, e-mail: dev-help@commons.apache.org
>  >>  >>
>  >>  >>
>  >>  >
>  >>  > ---------------------------------------------------------------------
>  >>  > To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>  >>  > For additional commands, e-mail: dev-help@commons.apache.org
>  >>  >
>  >>  >
>  >>
>  >>  ---------------------------------------------------------------------
>  >>  To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>  >>  For additional commands, e-mail: dev-help@commons.apache.org
>  >>
>  >>
>  >
>  > ---------------------------------------------------------------------
>  > To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>  > For additional commands, e-mail: dev-help@commons.apache.org
>  >
>  >
>
>  ---------------------------------------------------------------------
>  To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>  For additional commands, e-mail: dev-help@commons.apache.org
>
>

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org


Mime
View raw message