jmeter-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Philippe Mouawad <philippe.moua...@gmail.com>
Subject Re: svn commit: r1210091 - /jmeter/trunk/src/functions/org/apache/jmeter/functions/StringFromFile.java
Date Sun, 04 Dec 2011 19:59:37 GMT
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 <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
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message