You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@jmeter.apache.org by sebb <se...@gmail.com> on 2011/12/04 18:00:41 UTC
Re: svn commit: r1210091 - /jmeter/trunk/src/functions/org/apache/jmeter/functions/StringFromFile.java
On 4 December 2011 11:46, <pm...@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} */
>
>
Re: svn commit: r1210091 - /jmeter/trunk/src/functions/org/apache/jmeter/functions/StringFromFile.java
Posted by sebb <se...@gmail.com>.
On 5 December 2011 06:51, Philippe Mouawad <ph...@gmail.com> wrote:
> On Sunday, December 4, 2011, sebb <se...@gmail.com> wrote:
>> On 4 December 2011 19:59, Philippe Mouawad <ph...@gmail.com>
> wrote:
>>> testEnded is always called by one thread after all threads have ended no
> ?
>>
>> Yes, it is a different thread,
> From my understanding this call happens while all thread group threads have
> exited so I don' t see how there could be a corruption But maybe i missed
> something .
>>
>>> 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.
>>
>
> I agrée but in this particular case where is the issue ? Can you point it
> to me just for my full understanding ?
Test threads update the myBread field using synch. and also read it
while in synch. block. so they are OK.
The thread which runs testEnded() is different, and does not use
synch. to read the value when it calls close.
So it may see a stale value in the absence of any other synch.
Very unlikely, but not completely impossible.
>>> 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.
>>
> So for you this info log is part of the méthod contract ?
>>> But I let you decide.
>>
>> I will fix the safe publication issue.
>>
> I am ok with your fix.
>
>>> Regards
>>> Philippe
>>>
>>> On Sun, Dec 4, 2011 at 7:39 PM, sebb <se...@gmail.com> wrote:
>>>
>>>> On 4 December 2011 17:45, Philippe Mouawad <ph...@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 <se...@gmail.com> wrote:
>>>> >
>>>> >> On 4 December 2011 11:46, <pm...@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.apa
>
> --
> Cordialement.
> Philippe Mouawad.
Re: svn commit: r1210091 - /jmeter/trunk/src/functions/org/apache/jmeter/functions/StringFromFile.java
Posted by Philippe Mouawad <ph...@gmail.com>.
On Sunday, December 4, 2011, sebb <se...@gmail.com> wrote:
> On 4 December 2011 19:59, Philippe Mouawad <ph...@gmail.com>
wrote:
>> testEnded is always called by one thread after all threads have ended no
?
>
> Yes, it is a different thread,
>From my understanding this call happens while all thread group threads have
exited so I don' t see how there could be a corruption But maybe i missed
something .
>
>> 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.
>
I agrée but in this particular case where is the issue ? Can you point it
to me just for my full understanding ?
>> 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.
>
So for you this info log is part of the méthod contract ?
>> But I let you decide.
>
> I will fix the safe publication issue.
>
I am ok with your fix.
>> Regards
>> Philippe
>>
>> On Sun, Dec 4, 2011 at 7:39 PM, sebb <se...@gmail.com> wrote:
>>
>>> On 4 December 2011 17:45, Philippe Mouawad <ph...@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 <se...@gmail.com> wrote:
>>> >
>>> >> On 4 December 2011 11:46, <pm...@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.apa
--
Cordialement.
Philippe Mouawad.
Re: svn commit: r1210091 - /jmeter/trunk/src/functions/org/apache/jmeter/functions/StringFromFile.java
Posted by sebb <se...@gmail.com>.
On 4 December 2011 19:59, Philippe Mouawad <ph...@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 <se...@gmail.com> wrote:
>
>> On 4 December 2011 17:45, Philippe Mouawad <ph...@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 <se...@gmail.com> wrote:
>> >
>> >> On 4 December 2011 11:46, <pm...@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.
Re: svn commit: r1210091 - /jmeter/trunk/src/functions/org/apache/jmeter/functions/StringFromFile.java
Posted by Philippe Mouawad <ph...@gmail.com>.
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 <se...@gmail.com> wrote:
> On 4 December 2011 17:45, Philippe Mouawad <ph...@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 <se...@gmail.com> wrote:
> >
> >> On 4 December 2011 11:46, <pm...@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.
Re: svn commit: r1210091 - /jmeter/trunk/src/functions/org/apache/jmeter/functions/StringFromFile.java
Posted by sebb <se...@gmail.com>.
On 4 December 2011 17:45, Philippe Mouawad <ph...@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 <se...@gmail.com> wrote:
>
>> On 4 December 2011 11:46, <pm...@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.
Re: svn commit: r1210091 - /jmeter/trunk/src/functions/org/apache/jmeter/functions/StringFromFile.java
Posted by Philippe Mouawad <ph...@gmail.com>.
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 ?
Regards
Philipe
On Sun, Dec 4, 2011 at 6:00 PM, sebb <se...@gmail.com> wrote:
> On 4 December 2011 11:46, <pm...@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.