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 > > 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 >
