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 01:13:32 GMT
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.

>
>  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


Mime
View raw message