[
https://issues.apache.org/jira/browse/VELTOOLS-56?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12467060
]
Christopher Schultz commented on VELTOOLS-56:
---------------------------------------------
I'll run my tests once I get everything back together again. Merging the svn
project back into mine resulted in disaster due to the same patch being applied
twice but slightly differently, so I opted to start again. There's a lot of new
stuff in there. ;)
Now that I look at the diff, I can see what you meant by formatting issues.
Sorry... I had intended for much of that code to go into as small a space as
possible, hence the 2-line catch/handler kind of stuff. I generally format my
code so that the "real stuff" takes the most space, and the error handling is
as compact as possible. Same with the support class that I wrote... I didn't
think that a bunch of delegate methods needed to be expanded completely.
Forthcoming patches will be consistent with what I've seen, here.
> ImportSupport appears to have a veriety of problems, including a potential
> resource leak.
> -----------------------------------------------------------------------------------------
>
> Key: VELTOOLS-56
> URL: https://issues.apache.org/jira/browse/VELTOOLS-56
> Project: Velocity Tools
> Issue Type: Bug
> Affects Versions: 1.2
> Environment: Linux 2.4.28 + Java 1.4.2_09 + Tomcat 4.1.31
> Reporter: Christopher Schultz
> Fix For: 1.3
>
> Attachments: VELTOOLS-56.1.diff
>
>
> I have what I believe to be a relatively lean and mean search application
> that uses Velocity Tools, and after 2 months I started getting the dreaded
> OutOfMemoryException. I started looking for anything obvious that could be
> the problem, and I found that I was using ImportTool.read in a few places,
> and my log files were full of exceptions that occurred during those
> operations, because the URLs were bad or whatever.
> Just for the heck of it, I decided to look at how the ImportTool actually
> imported it's text, since it's a pretty heavy operation and if it was failing
> alot perhaps it was contributing to my OOM woes. So, I dove into the code to
> see how everything worked. The ImportTool class is trivial... the real meat
> is in ImportSupport, where I found some things that we might want to consider.
> 1. [trivial] In ImportSupport.acquireReader, an InputStreamReader is created
> with the character set loaded from the remote resource (makes sense). In the
> case where Java doesn't support the encoding, we use the default encoding.
> Unfortunately, the code uses catch(Exception) instead of
> catch(UnsupportedEncodingException). If some other error occurred, it would
> be swallowed, because there is no error reporting in this case.
> 2. [confusing] This comment appears just before the end of
> ImportSupport.acquireReader:
> // check response code for HTTP URLs before returning, per
> spec,
> // before returning
> It is unclear which spec the author is referencing. It could be the HTTP spec
> (unlikely) or the HttpURLConnection spec (where I can't find any hints). It
> might be nice to clear this up.
> Further, it appears that the author was trying to check the status in order
> to throw an error if there wasn't an "OK" response. This code appears /after/
> an input stream is retrieved from the request.
> It turns out that the class that you ultimately get back from opening an HTTP
> URL is sun.net.www.protocol.http.HttpURLConnection, which will throw an
> exception if you call getInputStream if the status of the connection is
> anything >= 400. That means that many of the error conditions are already
> handled by the time this status code check executes.
> Pretty much anything under 400 should be considered an okay response, no?
> 3. [potentially dangerous] There is never any resource cleanup of the HTTP
> connection. This is actually complicated due to the way that the methods were
> written: acquireReader creates the connection and returns a Reader object,
> which is tied to the HttpURLConnection. aquireString knows nothing about the
> HttpURLConnection and can therefore not cleanup. The reader itself gets
> closed, but that does not guarentee that the connection itself gets cleaned
> up.
> There's not even cleanup code in the acquireReader method itself to close the
> connection if we throw an exception ourselves (for example, because we don't
> like the status code).
> These problems leave potential, if not actual, resource leaks. Sure, the GC
> will hopefully come along eventually and cleanup the mess that was left, but
> it's not guarenteed to do so in a timely mannar. Things like InputStreams and
> HTTP connections are probably relatively heavy objects and should be cleaned
> up whenever possible.
> 4. [bad style?] All of the exceptions explicitly created and thrown from
> ImportSupport are of type java.lang.Exception. I feel like we could come up
> with some better exceptions to throw.
> I am happy to do the work required to fix all these issues -- I'm not just a
> whiner ;) I wanted to have a discussion about how the project leaders felt
> about each item and whether or not they were worth fixing.
> Thanks,
> -chris
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
---------------------------------------------------------------------
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
