Re: svn commit: r1690035 - in /tomcat/tc7.0.x/trunk: ./ java/org/apache/catalina/ant/ java/org/apache/catalina/ha/deploy/ webapps/docs/

2015-07-27 Thread Mark Thomas 
On 09/07/2015 18:50, Violeta Georgieva wrote:
 2015-07-09 15:56 GMT+03:00 Konstantin Kolinko knst.koli...@gmail.com:

snip/

 Reviewing DeployTask.java further (the code touched by
 http://svn.apache.org/r1689941 )

 For current trunk:

 http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/catalina/ant/DeployTask.java?view=markup#l156

snip/

 2. I wonder whether 2Gb file upload size limit here is needed.

 Maybe we can s/int contentLength/long contentLength/ and allow such
 big files.  There is a configurable file upload limit on Tomcat side
 (50Mb, in web.xml of manager webapp), though maybe it is not applied
 to PUT request used here.
 
 Yep but the restriction in manager app is only when multipart is used. Here
 this is not the case. So we can allow bigger files?

No objections in principle to larger files.

A limit above Integer.MAX_VALUE is going to require some minor API and
code changes to the Ant tasks as content length is currently handled as
an int. A quick look at the Manager code suggests that any size is
acceptable.

So, assuming the necessary int - long changes changes are made - no
objections here.

Mark


-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

Re: svn commit: r1690035 - in /tomcat/tc7.0.x/trunk: ./ java/org/apache/catalina/ant/ java/org/apache/catalina/ha/deploy/ webapps/docs/

2015-07-09 Thread Violeta Georgieva 
2015-07-09 15:56 GMT+03:00 Konstantin Kolinko knst.koli...@gmail.com:

 2015-07-09 15:29 GMT+03:00 Violeta Georgieva miles...@gmail.com:
  Hi,
 
  2015-07-09 14:50 GMT+03:00 Konstantin Kolinko knst.koli...@gmail.com:
 
  2015-07-09 12:06 GMT+03:00  violet...@apache.org:
   Author: violetagg
   Date: Thu Jul  9 09:06:25 2015
   New Revision: 1690035
  
   URL: http://svn.apache.org/r1690035
   Log:
   Merged revisions 1690011, 1690021 from tomcat/trunk:
   Fix possible resource leaks by closing streams properly. Issues
  reported by Coverity Scan.
  
   Modified:
   tomcat/tc7.0.x/trunk/   (props changed)
   tomcat/tc7.0.x/trunk/java/org/apache/catalina/ant/DeployTask.java
  
tomcat/tc7.0.x/trunk/java/org/apache/catalina/ant/ValidatorTask.java
  
 
tomcat/tc7.0.x/trunk/java/org/apache/catalina/ha/deploy/FarmWarDeployer.java
  
 
tomcat/tc7.0.x/trunk/java/org/apache/catalina/ha/deploy/LocalStrings.properties
   tomcat/tc7.0.x/trunk/webapps/docs/changelog.xml
  
  
   Modified:
  tomcat/tc7.0.x/trunk/java/org/apache/catalina/ant/DeployTask.java
   URL:
 
http://svn.apache.org/viewvc/tomcat/tc7.0.x/trunk/java/org/apache/catalina/ant/DeployTask.java?rev=1690035r1=1690034r2=1690035view=diff
  
 
==
   --- tomcat/tc7.0.x/trunk/java/org/apache/catalina/ant/DeployTask.java
  (original)
   +++ tomcat/tc7.0.x/trunk/java/org/apache/catalina/ant/DeployTask.java
  Thu Jul  9 09:06:25 2015
   @@ -199,6 +199,13 @@ public class DeployTask extends Abstract
sb.append(URLEncoder.encode(tag, getCharset()));
}
} catch (UnsupportedEncodingException e) {
   +if (stream != null) {
   +try {
   +stream.close();
   +} catch (IOException ioe) {
   +// Ignore
   +}
   +}
throw new BuildException(Invalid 'charset' attribute:
 +
  getCharset());
}
 
  The above is OK (it fixes an obvious error of non closing the stream
  when BuildException is thrown), but I think that it would be slightly
  better to close the stream in a finally{}. I mean:
 
  a) move execute(sb.toString(), stream, contentType, contentLength);
  line that follows the above lines into main try/catch block
  b) close the stream in a finally{}
 
  The good side that it will take care if there is an unexpected
  RuntimeException.
 
  It will change operation slightly that under normal conditions it will
  close the stream twice (once in execute() and second in finally()),
  but closing the same stream twice is allowed in Java.
 
 
 
http://docs.oracle.com/javase/6/docs/api/java/io/Closeable.html#close%28%29
  If the stream is already closed then invoking this method has no
effect.
 
 
  I agree.
  Feedback was applied.
 
  Thanks for the review,
  Violeta
 

 Thank you!

 Reviewing DeployTask.java further (the code touched by
 http://svn.apache.org/r1689941 )

 For current trunk:

http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/catalina/ant/DeployTask.java?view=markup#l156

 1. Line 156 and following:

 There is throw new UnsupportedOperationException(DeployTask does not
 support WAR files  + greater than 2 Gb);  lines.

 As java.lang.UnsupportedOperationException is not an IOException, a
 catch(IOException) block is not visited,so

 1) this error is not wrapped by BuildException
 2) fsInput stream is not closed.

 The stream variable can be s/BufferedInputStream/InputStream/ with
 further simplification of stream closing code, as execute() method
 does not expect a Buffered one.


 2. I wonder whether 2Gb file upload size limit here is needed.

 Maybe we can s/int contentLength/long contentLength/ and allow such
 big files.  There is a configurable file upload limit on Tomcat side
 (50Mb, in web.xml of manager webapp), though maybe it is not applied
 to PUT request used here.

Yep but the restriction in manager app is only when multipart is used. Here
this is not the case. So we can allow bigger files?

 Best regards,
 Konstantin Kolinko

 -
 To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
 For additional commands, e-mail: dev-h...@tomcat.apache.org

Re: svn commit: r1690035 - in /tomcat/tc7.0.x/trunk: ./ java/org/apache/catalina/ant/ java/org/apache/catalina/ha/deploy/ webapps/docs/

2015-07-09 Thread Konstantin Kolinko 
2015-07-09 12:06 GMT+03:00  violet...@apache.org:
 Author: violetagg
 Date: Thu Jul  9 09:06:25 2015
 New Revision: 1690035

 URL: http://svn.apache.org/r1690035
 Log:
 Merged revisions 1690011, 1690021 from tomcat/trunk:
 Fix possible resource leaks by closing streams properly. Issues reported by 
 Coverity Scan.

 Modified:
 tomcat/tc7.0.x/trunk/   (props changed)
 tomcat/tc7.0.x/trunk/java/org/apache/catalina/ant/DeployTask.java
 tomcat/tc7.0.x/trunk/java/org/apache/catalina/ant/ValidatorTask.java
 
 tomcat/tc7.0.x/trunk/java/org/apache/catalina/ha/deploy/FarmWarDeployer.java
 
 tomcat/tc7.0.x/trunk/java/org/apache/catalina/ha/deploy/LocalStrings.properties
 tomcat/tc7.0.x/trunk/webapps/docs/changelog.xml


 Modified: tomcat/tc7.0.x/trunk/java/org/apache/catalina/ant/DeployTask.java
 URL: 
 http://svn.apache.org/viewvc/tomcat/tc7.0.x/trunk/java/org/apache/catalina/ant/DeployTask.java?rev=1690035r1=1690034r2=1690035view=diff
 ==
 --- tomcat/tc7.0.x/trunk/java/org/apache/catalina/ant/DeployTask.java 
 (original)
 +++ tomcat/tc7.0.x/trunk/java/org/apache/catalina/ant/DeployTask.java Thu Jul 
  9 09:06:25 2015
 @@ -199,6 +199,13 @@ public class DeployTask extends Abstract
  sb.append(URLEncoder.encode(tag, getCharset()));
  }
  } catch (UnsupportedEncodingException e) {
 +if (stream != null) {
 +try {
 +stream.close();
 +} catch (IOException ioe) {
 +// Ignore
 +}
 +}
  throw new BuildException(Invalid 'charset' attribute:  + 
 getCharset());
  }

The above is OK (it fixes an obvious error of non closing the stream
when BuildException is thrown), but I think that it would be slightly
better to close the stream in a finally{}. I mean:

a) move execute(sb.toString(), stream, contentType, contentLength);
line that follows the above lines into main try/catch block
b) close the stream in a finally{}

The good side that it will take care if there is an unexpected RuntimeException.

It will change operation slightly that under normal conditions it will
close the stream twice (once in execute() and second in finally()),
but closing the same stream twice is allowed in Java.

http://docs.oracle.com/javase/6/docs/api/java/io/Closeable.html#close%28%29
If the stream is already closed then invoking this method has no effect.


The rest of this commit is OK, no comments.

Best regards,
Konstantin Kolinko

-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

Re: svn commit: r1690035 - in /tomcat/tc7.0.x/trunk: ./ java/org/apache/catalina/ant/ java/org/apache/catalina/ha/deploy/ webapps/docs/

2015-07-09 Thread Violeta Georgieva 
Hi,

2015-07-09 14:50 GMT+03:00 Konstantin Kolinko knst.koli...@gmail.com:

 2015-07-09 12:06 GMT+03:00  violet...@apache.org:
  Author: violetagg
  Date: Thu Jul  9 09:06:25 2015
  New Revision: 1690035
 
  URL: http://svn.apache.org/r1690035
  Log:
  Merged revisions 1690011, 1690021 from tomcat/trunk:
  Fix possible resource leaks by closing streams properly. Issues
reported by Coverity Scan.
 
  Modified:
  tomcat/tc7.0.x/trunk/   (props changed)
  tomcat/tc7.0.x/trunk/java/org/apache/catalina/ant/DeployTask.java
  tomcat/tc7.0.x/trunk/java/org/apache/catalina/ant/ValidatorTask.java
 
tomcat/tc7.0.x/trunk/java/org/apache/catalina/ha/deploy/FarmWarDeployer.java
 
tomcat/tc7.0.x/trunk/java/org/apache/catalina/ha/deploy/LocalStrings.properties
  tomcat/tc7.0.x/trunk/webapps/docs/changelog.xml
 
 
  Modified:
tomcat/tc7.0.x/trunk/java/org/apache/catalina/ant/DeployTask.java
  URL:
http://svn.apache.org/viewvc/tomcat/tc7.0.x/trunk/java/org/apache/catalina/ant/DeployTask.java?rev=1690035r1=1690034r2=1690035view=diff
 
==
  --- tomcat/tc7.0.x/trunk/java/org/apache/catalina/ant/DeployTask.java
(original)
  +++ tomcat/tc7.0.x/trunk/java/org/apache/catalina/ant/DeployTask.java
Thu Jul  9 09:06:25 2015
  @@ -199,6 +199,13 @@ public class DeployTask extends Abstract
   sb.append(URLEncoder.encode(tag, getCharset()));
   }
   } catch (UnsupportedEncodingException e) {
  +if (stream != null) {
  +try {
  +stream.close();
  +} catch (IOException ioe) {
  +// Ignore
  +}
  +}
   throw new BuildException(Invalid 'charset' attribute:  +
getCharset());
   }

 The above is OK (it fixes an obvious error of non closing the stream
 when BuildException is thrown), but I think that it would be slightly
 better to close the stream in a finally{}. I mean:

 a) move execute(sb.toString(), stream, contentType, contentLength);
 line that follows the above lines into main try/catch block
 b) close the stream in a finally{}

 The good side that it will take care if there is an unexpected
RuntimeException.

 It will change operation slightly that under normal conditions it will
 close the stream twice (once in execute() and second in finally()),
 but closing the same stream twice is allowed in Java.


http://docs.oracle.com/javase/6/docs/api/java/io/Closeable.html#close%28%29
 If the stream is already closed then invoking this method has no effect.


I agree.
Feedback was applied.

Thanks for the review,
Violeta


 The rest of this commit is OK, no comments.

 Best regards,
 Konstantin Kolinko

 -
 To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
 For additional commands, e-mail: dev-h...@tomcat.apache.org

Re: svn commit: r1690035 - in /tomcat/tc7.0.x/trunk: ./ java/org/apache/catalina/ant/ java/org/apache/catalina/ha/deploy/ webapps/docs/

2015-07-09 Thread Konstantin Kolinko 
2015-07-09 15:29 GMT+03:00 Violeta Georgieva miles...@gmail.com:
 Hi,

 2015-07-09 14:50 GMT+03:00 Konstantin Kolinko knst.koli...@gmail.com:

 2015-07-09 12:06 GMT+03:00  violet...@apache.org:
  Author: violetagg
  Date: Thu Jul  9 09:06:25 2015
  New Revision: 1690035
 
  URL: http://svn.apache.org/r1690035
  Log:
  Merged revisions 1690011, 1690021 from tomcat/trunk:
  Fix possible resource leaks by closing streams properly. Issues
 reported by Coverity Scan.
 
  Modified:
  tomcat/tc7.0.x/trunk/   (props changed)
  tomcat/tc7.0.x/trunk/java/org/apache/catalina/ant/DeployTask.java
  tomcat/tc7.0.x/trunk/java/org/apache/catalina/ant/ValidatorTask.java
 
 tomcat/tc7.0.x/trunk/java/org/apache/catalina/ha/deploy/FarmWarDeployer.java
 
 tomcat/tc7.0.x/trunk/java/org/apache/catalina/ha/deploy/LocalStrings.properties
  tomcat/tc7.0.x/trunk/webapps/docs/changelog.xml
 
 
  Modified:
 tomcat/tc7.0.x/trunk/java/org/apache/catalina/ant/DeployTask.java
  URL:
 http://svn.apache.org/viewvc/tomcat/tc7.0.x/trunk/java/org/apache/catalina/ant/DeployTask.java?rev=1690035r1=1690034r2=1690035view=diff
 
 ==
  --- tomcat/tc7.0.x/trunk/java/org/apache/catalina/ant/DeployTask.java
 (original)
  +++ tomcat/tc7.0.x/trunk/java/org/apache/catalina/ant/DeployTask.java
 Thu Jul  9 09:06:25 2015
  @@ -199,6 +199,13 @@ public class DeployTask extends Abstract
   sb.append(URLEncoder.encode(tag, getCharset()));
   }
   } catch (UnsupportedEncodingException e) {
  +if (stream != null) {
  +try {
  +stream.close();
  +} catch (IOException ioe) {
  +// Ignore
  +}
  +}
   throw new BuildException(Invalid 'charset' attribute:  +
 getCharset());
   }

 The above is OK (it fixes an obvious error of non closing the stream
 when BuildException is thrown), but I think that it would be slightly
 better to close the stream in a finally{}. I mean:

 a) move execute(sb.toString(), stream, contentType, contentLength);
 line that follows the above lines into main try/catch block
 b) close the stream in a finally{}

 The good side that it will take care if there is an unexpected
 RuntimeException.

 It will change operation slightly that under normal conditions it will
 close the stream twice (once in execute() and second in finally()),
 but closing the same stream twice is allowed in Java.


 http://docs.oracle.com/javase/6/docs/api/java/io/Closeable.html#close%28%29
 If the stream is already closed then invoking this method has no effect.


 I agree.
 Feedback was applied.

 Thanks for the review,
 Violeta


Thank you!

Reviewing DeployTask.java further (the code touched by
http://svn.apache.org/r1689941 )

For current trunk:
http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/catalina/ant/DeployTask.java?view=markup#l156

1. Line 156 and following:

There is throw new UnsupportedOperationException(DeployTask does not
support WAR files  + greater than 2 Gb);  lines.

As java.lang.UnsupportedOperationException is not an IOException, a
catch(IOException) block is not visited,so

1) this error is not wrapped by BuildException
2) fsInput stream is not closed.

The stream variable can be s/BufferedInputStream/InputStream/ with
further simplification of stream closing code, as execute() method
does not expect a Buffered one.


2. I wonder whether 2Gb file upload size limit here is needed.

Maybe we can s/int contentLength/long contentLength/ and allow such
big files.  There is a configurable file upload limit on Tomcat side
(50Mb, in web.xml of manager webapp), though maybe it is not applied
to PUT request used here.

Best regards,
Konstantin Kolinko

-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org