[back on list] On 24/05/13 15:06, Mauro Bartolomeoli wrote: > 2013/5/24 Ben Caradoc-Davies <ben.caradoc-dav...@csiro.au > <mailto:ben.caradoc-dav...@csiro.au>> > Mauro, I do not like the exception swallowing in > SchemaCache.download(). You changed to original: > throw new RuntimeException(e); > to > return null; > You are right, please have a look at my new commit: > https://github.com/mbarto/geotools/commit/d81f9f2dbaf549c6a0b47fda96bfef4bfccf2695 > I introduced a checked exception (IOException) so that I'm able to catch > it and call endDownload also in case of failure (this was the reason I > removed the throw in my first commit).
I don't understand. You changed this to a checked exception, but the only place you catch the exception you just chain and throw as a RuntimeException. Why do you need a checked exception? Unless I missed something, the cleanup is performed in the finally block, which is called for all exceptions, including unchecked, regardless of the contents of the catch. Please correct me if I am wrong or have misunderstood. Furthermore, if you want to use checked exceptions (which is a matter of taste and you are quite welcome to do so if you prefer), you can be even more consistent and catch IOException not Exception in download(); SocketTimoutException is a sub-sub-class of IOException. Either way should result in the same logic on timeout or other network failure. I also notice that you make extensive use of File.deleteOnExit(). I wonder if this is a best-practice as the list of files to be deleted may grow for a long-running application. Some reports suggest it lives in native memory. Others report deletion is unreliable. Ask Andrea, who is our expert on performance and memory leaks. http://puneeth.wordpress.com/2006/01/23/filedeleteonexit-is-evil/ http://www.pongasoft.com/blog/yan/java/2011/05/17/file-dot-deleteOnExit-is-evil/ Oh my, I am feeling like a grumpy old man today. :-) > I also updated the downloadTimeout to 60 seconds (for paranoids :-) ). Looks good! Feel free to merge into master as it is or with minor revisions. Do not give me a chance for more nitpicking. ;-) Kind regards, -- Ben Caradoc-Davies <ben.caradoc-dav...@csiro.au> Software Engineer CSIRO Earth Science and Resource Engineering Australian Resources Research Centre ------------------------------------------------------------------------------ Try New Relic Now & We'll Send You this Cool Shirt New Relic is the only SaaS-based application performance monitoring service that delivers powerful full stack analytics. Optimize and monitor your browser, app, & servers with just a few lines of code. Try New Relic and get this awesome Nerd Life shirt! http://p.sf.net/sfu/newrelic_d2d_may _______________________________________________ GeoTools-Devel mailing list GeoTools-Devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/geotools-devel
