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