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=1690035&r1=1690034&r2=1690035&view=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 >
