testEnded is always called by one thread after all threads have ended no ? So I think code is Ok, but it won't harm to add synchronized. My initial idea was in fact to remove closeFile as I thought logging was not part of a real contract (which is the difference with JOrphanUtils#closeQuietly()).
But I let you decide. Regards Philippe On Sun, Dec 4, 2011 at 7:39 PM, sebb <[email protected]> wrote: > On 4 December 2011 17:45, Philippe Mouawad <[email protected]> > wrote: > > Hello, > > This commit was related to inconsistent synchronization as it removed > code > > where inconsistent synchronisation occured (closeFile), in fact code is > OK > > but it seems a little inconsistent when only reading closeFile(). > > > > I hadn't noticed that closeFile differed from JOrphanUtils#closeQuietly > > only by logger.error, is it the behaviour you want to keep or I missed > > something ? > > Yes, you also missed the following log message. > > String tn = Thread.currentThread().getName(); > log.info(tn + " closing file " + fileName);//$NON-NLS-1$ > > A simpler solution might have been to make close synchronised. > Unfortunately your fix does not protect all accesses to myBread - > testEnded() also accesses myBread. > > > Regards > > Philipe > > > > On Sun, Dec 4, 2011 at 6:00 PM, sebb <[email protected]> wrote: > > > >> On 4 December 2011 11:46, <[email protected]> wrote: > >> > Author: pmouawad > >> > Date: Sun Dec 4 11:46:18 2011 > >> > New Revision: 1210091 > >> > > >> > URL: http://svn.apache.org/viewvc?rev=1210091&view=rev > >> > Log: > >> > Bug 52266 - Code:Inconsistent synchronization > >> > >> -1 > >> > >> Please don't make multiple unrelated changes in the same commit. > >> Sometimes it's OK to make other minor changes, but then all the > >> changes must be mentioned in the log message. > >> The commit log message should be enough for the reader to understand > >> the SVN diff e-mail. > >> > >> This commit also changes the close file behaviour. > >> The original behaviour was intended; why did you change it? > >> > >> Please restore the close file behaviour. > >> > >> > Modified: > >> > > >> > jmeter/trunk/src/functions/org/apache/jmeter/functions/StringFromFile.java > >> > > >> > Modified: > >> > jmeter/trunk/src/functions/org/apache/jmeter/functions/StringFromFile.java > >> > URL: > >> > http://svn.apache.org/viewvc/jmeter/trunk/src/functions/org/apache/jmeter/functions/StringFromFile.java?rev=1210091&r1=1210090&r2=1210091&view=diff > >> > > >> > ============================================================================== > >> > --- > >> > jmeter/trunk/src/functions/org/apache/jmeter/functions/StringFromFile.java > >> (original) > >> > +++ > >> > jmeter/trunk/src/functions/org/apache/jmeter/functions/StringFromFile.java > >> Sun Dec 4 11:46:18 2011 > >> > @@ -35,6 +35,7 @@ import org.apache.jmeter.threads.JMeterV > >> > import org.apache.jmeter.util.JMeterUtils; > >> > import org.apache.jorphan.logging.LoggingManager; > >> > import org.apache.jorphan.util.JMeterStopThreadException; > >> > +import org.apache.jorphan.util.JOrphanUtils; > >> > import org.apache.log.Logger; > >> > > >> > /** > >> > @@ -101,33 +102,13 @@ public class StringFromFile extends Abst > >> > private String fileName; // needed for error messages > >> > > >> > public StringFromFile() { > >> > - init(); > >> > + myValue = ERR_IND; > >> > + myName = "StringFromFile_";//$NON-NLS-1$ > >> > if (log.isDebugEnabled()) { > >> > log.debug("++++++++ Construct " + this); > >> > } > >> > } > >> > > >> > - private void init(){ > >> > - myValue = ERR_IND; > >> > - myName = "StringFromFile_";//$NON-NLS-1$ > >> > - } > >> > - > >> > - /** > >> > - * Close file and log > >> > - */ > >> > - private void closeFile() { > >> > - if (myBread == null) { > >> > - return; > >> > - } > >> > - String tn = Thread.currentThread().getName(); > >> > - log.info(tn + " closing file " + fileName);//$NON-NLS-1$ > >> > - try { > >> > - myBread.close(); > >> > - } catch (IOException e) { > >> > - log.error("closeFile() error: " + e.toString(), > >> e);//$NON-NLS-1$ > >> > - } > >> > - } > >> > - > >> > private static final int COUNT_UNUSED = -2; > >> > > >> > private int myStart = COUNT_UNUSED; > >> > @@ -232,7 +213,7 @@ public class StringFromFile extends Abst > >> > if (line == null) { // EOF, re-open file > >> > String tn = Thread.currentThread().getName(); > >> > log.info(tn + " EOF on file " + > >> fileName);//$NON-NLS-1$ > >> > - closeFile(); > >> > + JOrphanUtils.closeQuietly(myBread); > >> > openFile(); > >> > if (myBread != null) { > >> > line = myBread.readLine(); > >> > @@ -329,7 +310,7 @@ public class StringFromFile extends Abst > >> > > >> > /** {@inheritDoc} */ > >> > public void testEnded(String host) { > >> > - closeFile(); > >> > + JOrphanUtils.closeQuietly(myBread); > >> > } > >> > > >> > /** {@inheritDoc} */ > >> > > >> > > >> > > > > > > > > -- > > Cordialement. > > Philippe Mouawad. > -- Cordialement. Philippe Mouawad.
