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 Mon, 05 Dec 2011 09:37:15 GMT
On 5 December 2011 06:51, Philippe Mouawad <philippe.mouawad@gmail.com> wrote:
> On Sunday, December 4, 2011, sebb <sebbaz@gmail.com> wrote:
>> 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,
> 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 <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.apa
>
> --
> Cordialement.
> Philippe Mouawad.

Mime
View raw message