jmeter-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From sebb <seb...@gmail.com>
Subject Re: svn commit: r1210091 - /jmeter/trunk/src/functions/org/apache/jmeter/functions/StringFromFile.java
Date Sun, 04 Dec 2011 22:58:46 GMT
On 4 December 2011 19:59, Philippe Mouawad <philippe.mouawad@gmail.com> wrote:
> testEnded is always called by one thread after all threads have ended no ?

Yes, it is a different thread,

> So I think code is Ok, but it won't harm to add synchronized.

No, the code is not OK as it stands.

That is because of the Java memory model which allows threads to cache data.
In the absence of synch (or volatile), an updated value written by one
thread may perhaps never be seen by another thread.
Also updates can be seen out of order.
Some JVMs may even use 2 writes to update a long, in which case other
threads may see half old/half new values.

> 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()).

That was part of the difference in the close() method; the code also
logged that the file was about to be closed.

> But I let you decide.

I will fix the safe publication issue.

> Regards
> Philippe
>
> On Sun, Dec 4, 2011 at 7:39 PM, sebb <sebbaz@gmail.com> wrote:
>
>> On 4 December 2011 17:45, Philippe Mouawad <philippe.mouawad@gmail.com>
>> 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 <sebbaz@gmail.com> wrote:
>> >
>> >> On 4 December 2011 11:46,  <pmouawad@apache.org> 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.

Mime
View raw message