You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@jmeter.apache.org by pm...@apache.org on 2011/11/30 23:16:36 UTC
svn commit: r1208836 -
/jmeter/trunk/src/core/org/apache/jmeter/reporters/ResultCollector.java
Author: pmouawad
Date: Wed Nov 30 22:16:35 2011
New Revision: 1208836
URL: http://svn.apache.org/viewvc?rev=1208836&view=rev
Log:
Made code cleaner
Modified:
jmeter/trunk/src/core/org/apache/jmeter/reporters/ResultCollector.java
Modified: jmeter/trunk/src/core/org/apache/jmeter/reporters/ResultCollector.java
URL: http://svn.apache.org/viewvc/jmeter/trunk/src/core/org/apache/jmeter/reporters/ResultCollector.java?rev=1208836&r1=1208835&r2=1208836&view=diff
==============================================================================
--- jmeter/trunk/src/core/org/apache/jmeter/reporters/ResultCollector.java (original)
+++ jmeter/trunk/src/core/org/apache/jmeter/reporters/ResultCollector.java Wed Nov 30 22:16:35 2011
@@ -389,10 +389,10 @@ public class ResultCollector extends Abs
// Find the name of the directory containing the file
// and create it - if there is one
File pdir = new File(filename).getParentFile();
- if (pdir != null) {
- pdir.mkdirs();// returns false if directory already exists, so need to check again
- if (!pdir.exists()){
- log.warn("Error creating directories for "+pdir.toString());
+ if (pdir != null && !pdir.exists()) {
+ boolean mkdirResult = pdir.mkdirs();
+ if (!mkdirResult){
+ log.warn("Error creating directories for "+pdir.getAbsolutePath());
}
}
writer = new PrintWriter(new OutputStreamWriter(new BufferedOutputStream(new FileOutputStream(filename,
Re: svn commit: r1208836 - /jmeter/trunk/src/core/org/apache/jmeter/reporters/ResultCollector.java
Posted by sebb <se...@gmail.com>.
On 1 December 2011 08:59, Philippe Mouawad <ph...@gmail.com> wrote:
> Hello Sebb,
> I checked code before doing this and method is called from
> <initializeFileOutput (private) < testStarted (public) in synchronized
> block so it is OK like this as no other method calls it.
No, it's not OK. - synch. does not help here.
Some external process can change the directory structure.
Highly unlikely, but not impossible.
> But if you think it should be as it was, I can rollback.
Yes, please do.
> Regards
> Philippe
>
> On Thu, Dec 1, 2011 at 1:01 AM, sebb <se...@gmail.com> wrote:
>
>> On 30 November 2011 22:16, <pm...@apache.org> wrote:
>> > Author: pmouawad
>> > Date: Wed Nov 30 22:16:35 2011
>> > New Revision: 1208836
>> >
>> > URL: http://svn.apache.org/viewvc?rev=1208836&view=rev
>> > Log:
>> > Made code cleaner
>>
>> Perhaps, but it introduces a subtle bug, see below.
>>
>> > Modified:
>> > jmeter/trunk/src/core/org/apache/jmeter/reporters/ResultCollector.java
>> >
>> > Modified:
>> jmeter/trunk/src/core/org/apache/jmeter/reporters/ResultCollector.java
>> > URL:
>> http://svn.apache.org/viewvc/jmeter/trunk/src/core/org/apache/jmeter/reporters/ResultCollector.java?rev=1208836&r1=1208835&r2=1208836&view=diff
>> >
>> ==============================================================================
>> > ---
>> jmeter/trunk/src/core/org/apache/jmeter/reporters/ResultCollector.java
>> (original)
>> > +++
>> jmeter/trunk/src/core/org/apache/jmeter/reporters/ResultCollector.java Wed
>> Nov 30 22:16:35 2011
>> > @@ -389,10 +389,10 @@ public class ResultCollector extends Abs
>> > // Find the name of the directory containing the file
>> > // and create it - if there is one
>> > File pdir = new File(filename).getParentFile();
>> > - if (pdir != null) {
>> > - pdir.mkdirs();// returns false if directory already
>> exists, so need to check again
>> > - if (!pdir.exists()){
>> > - log.warn("Error creating directories for
>> "+pdir.toString());
>> > + if (pdir != null && !pdir.exists()) {
>> > + boolean mkdirResult = pdir.mkdirs();
>> > + if (!mkdirResult){
>>
>> It's not totally safe to check the return from mkdirs() after checking
>> exists().
>> See for example:
>>
>> https://issues.apache.org/jira/browse/JCI-67
>>
>> Please revert the change.
>>
>> > + log.warn("Error creating directories for
>> "+pdir.getAbsolutePath());
>> > }
>> > }
>> > writer = new PrintWriter(new OutputStreamWriter(new
>> BufferedOutputStream(new FileOutputStream(filename,
>> >
>> >
>>
>
>
>
> --
> Cordialement.
> Philippe Mouawad.
Re: svn commit: r1208836 - /jmeter/trunk/src/core/org/apache/jmeter/reporters/ResultCollector.java
Posted by Philippe Mouawad <ph...@gmail.com>.
Hello Sebb,
I checked code before doing this and method is called from
<initializeFileOutput (private) < testStarted (public) in synchronized
block so it is OK like this as no other method calls it.
But if you think it should be as it was, I can rollback.
Regards
Philippe
On Thu, Dec 1, 2011 at 1:01 AM, sebb <se...@gmail.com> wrote:
> On 30 November 2011 22:16, <pm...@apache.org> wrote:
> > Author: pmouawad
> > Date: Wed Nov 30 22:16:35 2011
> > New Revision: 1208836
> >
> > URL: http://svn.apache.org/viewvc?rev=1208836&view=rev
> > Log:
> > Made code cleaner
>
> Perhaps, but it introduces a subtle bug, see below.
>
> > Modified:
> > jmeter/trunk/src/core/org/apache/jmeter/reporters/ResultCollector.java
> >
> > Modified:
> jmeter/trunk/src/core/org/apache/jmeter/reporters/ResultCollector.java
> > URL:
> http://svn.apache.org/viewvc/jmeter/trunk/src/core/org/apache/jmeter/reporters/ResultCollector.java?rev=1208836&r1=1208835&r2=1208836&view=diff
> >
> ==============================================================================
> > ---
> jmeter/trunk/src/core/org/apache/jmeter/reporters/ResultCollector.java
> (original)
> > +++
> jmeter/trunk/src/core/org/apache/jmeter/reporters/ResultCollector.java Wed
> Nov 30 22:16:35 2011
> > @@ -389,10 +389,10 @@ public class ResultCollector extends Abs
> > // Find the name of the directory containing the file
> > // and create it - if there is one
> > File pdir = new File(filename).getParentFile();
> > - if (pdir != null) {
> > - pdir.mkdirs();// returns false if directory already
> exists, so need to check again
> > - if (!pdir.exists()){
> > - log.warn("Error creating directories for
> "+pdir.toString());
> > + if (pdir != null && !pdir.exists()) {
> > + boolean mkdirResult = pdir.mkdirs();
> > + if (!mkdirResult){
>
> It's not totally safe to check the return from mkdirs() after checking
> exists().
> See for example:
>
> https://issues.apache.org/jira/browse/JCI-67
>
> Please revert the change.
>
> > + log.warn("Error creating directories for
> "+pdir.getAbsolutePath());
> > }
> > }
> > writer = new PrintWriter(new OutputStreamWriter(new
> BufferedOutputStream(new FileOutputStream(filename,
> >
> >
>
--
Cordialement.
Philippe Mouawad.
Re: svn commit: r1208836 - /jmeter/trunk/src/core/org/apache/jmeter/reporters/ResultCollector.java
Posted by sebb <se...@gmail.com>.
On 30 November 2011 22:16, <pm...@apache.org> wrote:
> Author: pmouawad
> Date: Wed Nov 30 22:16:35 2011
> New Revision: 1208836
>
> URL: http://svn.apache.org/viewvc?rev=1208836&view=rev
> Log:
> Made code cleaner
Perhaps, but it introduces a subtle bug, see below.
> Modified:
> jmeter/trunk/src/core/org/apache/jmeter/reporters/ResultCollector.java
>
> Modified: jmeter/trunk/src/core/org/apache/jmeter/reporters/ResultCollector.java
> URL: http://svn.apache.org/viewvc/jmeter/trunk/src/core/org/apache/jmeter/reporters/ResultCollector.java?rev=1208836&r1=1208835&r2=1208836&view=diff
> ==============================================================================
> --- jmeter/trunk/src/core/org/apache/jmeter/reporters/ResultCollector.java (original)
> +++ jmeter/trunk/src/core/org/apache/jmeter/reporters/ResultCollector.java Wed Nov 30 22:16:35 2011
> @@ -389,10 +389,10 @@ public class ResultCollector extends Abs
> // Find the name of the directory containing the file
> // and create it - if there is one
> File pdir = new File(filename).getParentFile();
> - if (pdir != null) {
> - pdir.mkdirs();// returns false if directory already exists, so need to check again
> - if (!pdir.exists()){
> - log.warn("Error creating directories for "+pdir.toString());
> + if (pdir != null && !pdir.exists()) {
> + boolean mkdirResult = pdir.mkdirs();
> + if (!mkdirResult){
It's not totally safe to check the return from mkdirs() after checking exists().
See for example:
https://issues.apache.org/jira/browse/JCI-67
Please revert the change.
> + log.warn("Error creating directories for "+pdir.getAbsolutePath());
> }
> }
> writer = new PrintWriter(new OutputStreamWriter(new BufferedOutputStream(new FileOutputStream(filename,
>
>