Hi all,
I tried to fix some remaining issues with GEOT-4467, in particular the
File.deleteOnExit() usage.
You find the alternative solution here:
https://github.com/mbarto/geotools/commit/c1733941f9cfaf3642e7ce33cf8844b4ae21b104
It's based on synchronization in the topic moments of resolving from cache,
that are:
- checking for cached schema existance on disk before downloading it
- storing the downloaded file to disk
Synchronization is used to avoid returning a partially written file.
A check is introduced to not write a schema to disk after download, if it
already exists (downloaded from another thread).
Synchronization time should be short (the time to write a schema file on
disk).
Let me know if this approach seems better or worse than the previous one.
Mauro
2013/5/24 Ben Caradoc-Davies <[email protected]>
> [back on list]
>
> On 24/05/13 15:06, Mauro Bartolomeoli wrote:
>
>> 2013/5/24 Ben Caradoc-Davies <[email protected]
>> <mailto:Ben.Caradoc-Davies@**csiro.au <[email protected]>>>
>>
>> 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/**
>> d81f9f2dbaf549c6a0b47fda96bfef**4bfccf2695<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://puneeth.wordpress.com/2006/01/23/filedeleteonexit-is-evil/>
> http://www.pongasoft.com/blog/**yan/java/2011/05/17/file-dot-**
> deleteOnExit-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 <[email protected]>
> Software Engineer
> CSIRO Earth Science and Resource Engineering
> Australian Resources Research Centre
>
--
==
GeoServer training in Milan, 6th & 7th June 2013! Visit
http://geoserver.geo-solutions.it for more information.
==
Dott. Mauro Bartolomeoli
@mauro_bart
Senior Software Engineer
GeoSolutions S.A.S.
Via Poggio alle Viti 1187
55054 Massarosa (LU)
Italy
phone: +39 0584 962313
fax: +39 0584 1660272
http://www.geo-solutions.it
http://twitter.com/geosolutions_it
-------------------------------------------------------
------------------------------------------------------------------------------
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
[email protected]
https://lists.sourceforge.net/lists/listinfo/geotools-devel
