Bug 48771

Summary:	[PATCH] CleanerThread never terminates, causes leaks in webapp
Product:	Batik - Now in Jira	Reporter:	Joel Carranza <jec>
Component:	Utilities	Assignee:	Batik Developer's Mailing list <batik-dev>
Status:	NEW ---
Severity:	normal	Keywords:	PatchAvailable
Priority:	P2
Version:	1.7
Target Milestone:	---
Hardware:	PC
OS:	Windows XP
URL:	http://mail-archives.apache.org/mod_mbox/xmlgraphics-batik-users/201002.mbox/%3C27631790.post@talk.nabble.com%3E
Attachments:	Exposes an exit method in CleanerThread Patch updated to conform to style guidelines

Description Joel Carranza 2010-02-18 19:55:23 UTC

Created attachment 25020 [details]
Exposes an exit method in CleanerThread

org.apache.batik.util.CleanerThread does not self-terminate, nor is there a way to dispose of it programatically. When using Batik within a webapp, this prevents the webapp's classloader from being garbage collected, eventually causing PermGen: Out of Memory errors after several webapp redeploys. CleanerThread should expose some method that allows for it to be terminated.

Patch attached which follows the same solution used in geotools package [https://fisheye.codehaus.org/browse/GEOT-2742]

Comment 1 Helder Magalhães 2010-02-19 07:23:10 UTC

Thanks for taking the time to investigate/share the patch, Joel! :-)

Without testing it, I think it would be interesting to mention that there was some discussion about this in the mailing list prior to this bug report being created. I've added a link to in as the issue's URL. ;-)

I'm also adding metadata to reflect that a patch was made available.

Should/can this be related to bug 44178? At least, one may be interested in reviewing both issues together... :-)


A (somehow) quick review of the patch, mostly with cosmetic nits, follows.


> +    public static final CleanerThread  THREAD = new CleanerThread();

Double space before "THREAD".


> +        queue = new ReferenceQueue<Object>();

We're not using generics yet, as Batik still targets Java 1.4 compatibility AFAIK (see bug 46434). Could this be reworked?


> +    	ReferenceQueue<Object> rq;

Tab character here. Indentation is made using 4 spaces within the framework.


> +        } catch (InterruptedException e) {
> +
> +        }

I'd say that either insert a comment, stating why the exception is ignored (if it matters), or get rid of the extraneous line.

Comment 2 Joel Carranza 2010-02-19 16:59:57 UTC

Created attachment 25025 [details]
Patch updated to conform to style guidelines

Comment 3 Joel Carranza 2010-02-19 17:01:34 UTC

I would claim that the proposed patch also solves the race condition described in 44178

Comment 4 Helder Magalhães 2010-02-19 21:45:42 UTC

(In reply to comment #2)
> Created an attachment (id=25025) [details]
> Patch updated to conform to style guidelines

I've taken a peek and it looks good, thanks. :-)


(In reply to comment #3)
> I would claim that the proposed patch also solves the race condition described
> in 44178

Great! ;-) This appears to be quite safe and potentially fixes a couple issues. Still, I'd ask for a third opinion/deeper review by a more elder committer (Thomas? Cameron? Dieter?)

Comment 5 kgo1177 2011-07-14 15:13:48 UTC

Hi everybody,

Any chance that the proposed (or a similar) solution would become part of an official release any time soon? We use Batik quite a bit in a web app to export SVG and derived formats and are currently trying to get rid of a lot of memory leaks, so that we don't have to restart Tomcat regularly...

Cheers,