On 24/05/2008, Niall Pemberton <[EMAIL PROTECTED]> wrote: > On Sat, May 24, 2008 at 2:13 AM, sebb <[EMAIL PROTECTED]> wrote: > > On 24/05/2008, Niall Pemberton <[EMAIL PROTECTED]> wrote: > >> On Fri, May 23, 2008 at 11:56 PM, sebb <[EMAIL PROTECTED]> wrote: > >> > On 23/05/2008, Niall Pemberton <[EMAIL PROTECTED]> wrote: > >> >> On Fri, May 23, 2008 at 7:17 PM, sebb <[EMAIL PROTECTED]> wrote: > >> >> > On 23/05/2008, Niall Pemberton <[EMAIL PROTECTED]> wrote: > >> >> >> On Fri, May 23, 2008 at 4:51 PM, sebb <[EMAIL PROTECTED]> wrote: > >> >> >> > On 23/05/2008, Niall Pemberton <[EMAIL PROTECTED]> wrote: > >> >> >> >> On Fri, May 23, 2008 at 3:31 PM, Luc Maisonobe <[EMAIL > PROTECTED]> wrote: > >> >> >> >> > Niall Pemberton a écrit : > >> >> >> >> >> On Thu, May 22, 2008 at 9:35 PM, Luc Maisonobe <[EMAIL > PROTECTED]> 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: [EMAIL PROTECTED] > >> >> >> For additional commands, e-mail: [EMAIL PROTECTED] > >> >> >> > >> >> >> > >> >> > > >> >> > > --------------------------------------------------------------------- > >> >> > To unsubscribe, e-mail: [EMAIL PROTECTED] > >> >> > For additional commands, e-mail: [EMAIL PROTECTED] > >> >> > > >> >> > > >> >> > >> >> --------------------------------------------------------------------- > >> >> To unsubscribe, e-mail: [EMAIL PROTECTED] > >> >> For additional commands, e-mail: [EMAIL PROTECTED] > >> >> > >> >> > >> > > >> > --------------------------------------------------------------------- > >> > To unsubscribe, e-mail: [EMAIL PROTECTED] > >> > For additional commands, e-mail: [EMAIL PROTECTED] > >> > > >> > > >> > >> --------------------------------------------------------------------- > >> To unsubscribe, e-mail: [EMAIL PROTECTED] > >> For additional commands, e-mail: [EMAIL PROTECTED] > >> > >> > > > > --------------------------------------------------------------------- > > To unsubscribe, e-mail: [EMAIL PROTECTED] > > For additional commands, e-mail: [EMAIL PROTECTED] > > > > > > --------------------------------------------------------------------- > To unsubscribe, e-mail: [EMAIL PROTECTED] > For additional commands, e-mail: [EMAIL PROTECTED] > > --------------------------------------------------------------------- To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
