Обсуждение: non-trivial finalize() on AbstractJdbc2Statement

Well this is a typical which is worse case scenario. People leaking
resources because they don't explicitly close them, or your case ? I'm
not sure which is worse.

Given that the driver is being used in many very high throughput sites
without this problem, I'm curious as to why nobody else has complained

Dave Cramer

dave.cramer(at)credativ(dot)ca
http://www.credativ.ca

On Fri, Feb 10, 2012 at 12:39 PM, Imran <imranbohoran@gmail.com> wrote:
> Hello
>
> We've been having OOM errors in our applications through GC overhead limits
> under heave load resources running queries. Inspecting the heap dump, it
> appears that the finalizer queue is taken up most of the heap space. Almost
> all of the the finalizer objects I've seen seem to have a
> jdbc3PreparedStatement object in it. Going through the source code of the
> driver I see that the 'AbstractJdbc2Statement' has a non-trivial finalize
> method. I guess this explains why these objects end up in the finalizer
> queue. Can I clarify the need to having this finalize() method here? It
> seems to be calling the close() method of the statement which I would have
> thought is the responsibility of the client building a Statement object. Is
> there any chance this can be dropped so we don't see these objects ending up
> in the finalizer queue under heavy load and the jvm running out of memory
> before the GC threads gets around to 'actually' reclaim the memory?
>
> Also we are using postgres 9.0.4 and the 8.3-604.jdbc3 version of the
> postgresql jdbc driver.
>
> Cheers
> -- Imran

Hello.

While it would be better to handle resource closing with reference queues, are you sure your client does not leave resources unclosed? As for me, driver in finalize should do only work not done because of close not have been called correctly.  In other words, your situation may be an indication of close not been called, and so driver does required work in finalize, so saturating finalize thread.

Best regards, Vitalii Tymchyshyn

13.02.12 15:02, Imran написав(ла):

Hi Dave

Thanks for your response. I agree that this might not be widespread. And I haven't seen much mentions around this problem (hitting GC issues due to non-trivial finalize() methods) reported (based on my google searches). However, I guess the existence of it does provide the opportunity for this to happen as we've noticed. IMHO, encouraging clients to use the api as recommended (i.e. closing resources once they are done with them) is better than the driver allowing opportunity for bad things to happen. Or perhaps the driver caters to this kind of situation using WeakReference as oppose to using a non-trivial finalize(). 

What are your thoughts?

Cheers

-- Imran  

On Mon, Feb 13, 2012 at 12:09 PM, Dave Cramer <pg@fastcrypt.com> wrote:

Well this is a typical which is worse case scenario. People leaking
resources because they don't explicitly close them, or your case ? I'm
not sure which is worse.

Given that the driver is being used in many very high throughput sites
without this problem, I'm curious as to why nobody else has complained

Dave Cramer

dave.cramer(at)credativ(dot)ca
http://www.credativ.ca

On Fri, Feb 10, 2012 at 12:39 PM, Imran <imranbohoran@gmail.com> wrote:
> Hello
>
> We've been having OOM errors in our applications through GC overhead limits
> under heave load resources running queries. Inspecting the heap dump, it
> appears that the finalizer queue is taken up most of the heap space. Almost
> all of the the finalizer objects I've seen seem to have a
> jdbc3PreparedStatement object in it. Going through the source code of the
> driver I see that the 'AbstractJdbc2Statement' has a non-trivial finalize
> method. I guess this explains why these objects end up in the finalizer
> queue. Can I clarify the need to having this finalize() method here? It
> seems to be calling the close() method of the statement which I would have
> thought is the responsibility of the client building a Statement object. Is
> there any chance this can be dropped so we don't see these objects ending up
> in the finalizer queue under heavy load and the jvm running out of memory
> before the GC threads gets around to 'actually' reclaim the memory?
>
> Also we are using postgres 9.0.4 and the 8.3-604.jdbc3 version of the
> postgresql jdbc driver.
>
> Cheers
> -- Imran

Hi Vitalli

Yes my clients are definitely closing the PreparedStatements opened. In fact we are using the hibernate query interface and I've been through the code to make sure the close is been called in a finally block.

I agree that closing work done by the finalizer() method would actually close the resource if its not closed. However the issue I'm raising is that by having the non-trivial finalize() method, it adds every PreparedStatement object created into the finalizer queue. And hence the a GC not reclaiming all memory is should. So before the finalizer thread (which is a low priority thread) gets to it, the application has ran out of memory.

Cheers

-- Imran 

On Mon, Feb 13, 2012 at 1:28 PM, Vitalii Tymchyshyn <tivv00@gmail.com> wrote:

Hello.

While it would be better to handle resource closing with reference queues, are you sure your client does not leave resources unclosed? As for me, driver in finalize should do only work not done because of close not have been called correctly.  In other words, your situation may be an indication of close not been called, and so driver does required work in finalize, so saturating finalize thread.

Best regards, Vitalii Tymchyshyn

13.02.12 15:02, Imran написав(ла):

Hi Dave

Thanks for your response. I agree that this might not be widespread. And I haven't seen much mentions around this problem (hitting GC issues due to non-trivial finalize() methods) reported (based on my google searches). However, I guess the existence of it does provide the opportunity for this to happen as we've noticed. IMHO, encouraging clients to use the api as recommended (i.e. closing resources once they are done with them) is better than the driver allowing opportunity for bad things to happen. Or perhaps the driver caters to this kind of situation using WeakReference as oppose to using a non-trivial finalize(). 

What are your thoughts?

Cheers

-- Imran  

On Mon, Feb 13, 2012 at 12:09 PM, Dave Cramer <pg@fastcrypt.com> wrote:

Well this is a typical which is worse case scenario. People leaking
resources because they don't explicitly close them, or your case ? I'm
not sure which is worse.

Given that the driver is being used in many very high throughput sites
without this problem, I'm curious as to why nobody else has complained

Dave Cramer

dave.cramer(at)credativ(dot)ca
http://www.credativ.ca

On Fri, Feb 10, 2012 at 12:39 PM, Imran <imranbohoran@gmail.com> wrote:
> Hello
>
> We've been having OOM errors in our applications through GC overhead limits
> under heave load resources running queries. Inspecting the heap dump, it
> appears that the finalizer queue is taken up most of the heap space. Almost
> all of the the finalizer objects I've seen seem to have a
> jdbc3PreparedStatement object in it. Going through the source code of the
> driver I see that the 'AbstractJdbc2Statement' has a non-trivial finalize
> method. I guess this explains why these objects end up in the finalizer
> queue. Can I clarify the need to having this finalize() method here? It
> seems to be calling the close() method of the statement which I would have
> thought is the responsibility of the client building a Statement object. Is
> there any chance this can be dropped so we don't see these objects ending up
> in the finalizer queue under heavy load and the jvm running out of memory
> before the GC threads gets around to 'actually' reclaim the memory?
>
> Also we are using postgres 9.0.4 and the 8.3-604.jdbc3 version of the
> postgresql jdbc driver.
>
> Cheers
> -- Imran

Hi Vitalli

Yes my clients are definitely closing the PreparedStatements opened. In fact we are using the hibernate query interface and I've been through the code to make sure the close is been called in a finally block.

I agree that closing work done by the finalizer() method would actually close the resource if its not closed. However the issue I'm raising is that by having the non-trivial finalize() method, it adds every PreparedStatement object created into the finalizer queue. And hence the a GC not reclaiming all memory is should. So before the finalizer thread (which is a low priority thread) gets to it, the application has ran out of memory.

Cheers

-- Imran 

On Mon, Feb 13, 2012 at 1:28 PM, Vitalii Tymchyshyn <tivv00@gmail.com> wrote:

Hello.

While it would be better to handle resource closing with reference queues, are you sure your client does not leave resources unclosed? As for me, driver in finalize should do only work not done because of close not have been called correctly.  In other words, your situation may be an indication of close not been called, and so driver does required work in finalize, so saturating finalize thread.

Best regards, Vitalii Tymchyshyn

13.02.12 15:02, Imran написав(ла):

Hi Dave

Thanks for your response. I agree that this might not be widespread. And I haven't seen much mentions around this problem (hitting GC issues due to non-trivial finalize() methods) reported (based on my google searches). However, I guess the existence of it does provide the opportunity for this to happen as we've noticed. IMHO, encouraging clients to use the api as recommended (i.e. closing resources once they are done with them) is better than the driver allowing opportunity for bad things to happen. Or perhaps the driver caters to this kind of situation using WeakReference as oppose to using a non-trivial finalize(). 

What are your thoughts?

Cheers

-- Imran  

On Mon, Feb 13, 2012 at 12:09 PM, Dave Cramer <pg@fastcrypt.com> wrote:

Well this is a typical which is worse case scenario. People leaking
resources because they don't explicitly close them, or your case ? I'm
not sure which is worse.

Given that the driver is being used in many very high throughput sites
without this problem, I'm curious as to why nobody else has complained

Dave Cramer

dave.cramer(at)credativ(dot)ca
http://www.credativ.ca

On Fri, Feb 10, 2012 at 12:39 PM, Imran <imranbohoran@gmail.com> wrote:
> Hello
>
> We've been having OOM errors in our applications through GC overhead limits
> under heave load resources running queries. Inspecting the heap dump, it
> appears that the finalizer queue is taken up most of the heap space. Almost
> all of the the finalizer objects I've seen seem to have a
> jdbc3PreparedStatement object in it. Going through the source code of the
> driver I see that the 'AbstractJdbc2Statement' has a non-trivial finalize
> method. I guess this explains why these objects end up in the finalizer
> queue. Can I clarify the need to having this finalize() method here? It
> seems to be calling the close() method of the statement which I would have
> thought is the responsibility of the client building a Statement object. Is
> there any chance this can be dropped so we don't see these objects ending up
> in the finalizer queue under heavy load and the jvm running out of memory
> before the GC threads gets around to 'actually' reclaim the memory?
>
> Also we are using postgres 9.0.4 and the 8.3-604.jdbc3 version of the
> postgresql jdbc driver.
>
> Cheers
> -- Imran

******* ********<tivv00@gmail.com> wrote:
> Looking at the code... Can it be because
> org.postgresql.jdbc2.AbstractJdbc2Statement#isClosed is not
> volatile? There is no synchronization and finalizer thread may
> simply not see statement was just closed by another thread.

That sounds likely enough to me.  I don't know whether declaring the
flag volatile would be enough, but it needs either that or access
only through synchronized blocks.

In addition, I would recommend something like the attached to make
the code more bullet-proof.

-Kevin

On Wed, Feb 15, 2012 at 6:30 AM, Vitalii Tymchyshyn <tivv00@gmail.com> wrote:
> Hello.
>
> The finalizer thread is not a low priority thread in my JVM (openjdk) and as
> soon as Statement.close does nothing (and it does nothing if statement is
> closed), I don't see how can it be a problem to free memory. Actually
> statement creation is much more heavy thing than "if (flag) return". It's
> perfectly valid to close native resource (and as far as I know statement
> allocates server-side native resources) in finalizer. Another implementation
> can be to create a ReferenceQueue and a bunch of References that should be
> checked now and then during connection calls, but this introduces additional
> processing that is not needed most time. You can't drop this code altogether
> as while it's good practice to close statements in client code, you can't
> guarantee it's done everywhere. Actually it's common practice in java to
> clean-up external resources in finalize, see for example FileOutputStream.
> As of your problem, I don't thing statement finalize method is the problem
> cause. I can see two causes:
> 1) The code bug mentioned in this thread
> 2) Some another class with long finalize method. This is the case that can
> lead to confusion. Say, you have an object created once in 5 minutes that
> has 3 minutes finalize. It won't hurt by itself as it's enough time to be
> finalized. But as soon as you have additional object with finalized method
> defined (say, Statement), even empty, all this objects can't be freed during
> this 3 minutes run and you will get OOM. And you will rarely blame your
> custom object. The best check that can be done to show real reason is to
> check Finalizer object stack with jstack. I did often see cases when custom
> finalize method is fast, but is synchronized by some heavy-used  resource.
>
> Best regards, Vitalii Tymchyshyn

Just for kicks why don't you remove the finalize method recompile the
JDBC jar and try it.

Dave Cramer

dave.cramer(at)credativ(dot)ca
http://www.credativ.ca

2012/2/15 Imran <imranbohoran@gmail.com>

So I've tried to run jstack over five times and it kept in saying it can't walk over the threads. We have a fair amount of background processes running, not sure if that made jstack fail. I've read that its possible to hit the error on jstack. My thread dumps from jvisualvm doesn't show the priority of any threads.

I have visualvm 1.3. I can select application in the list -> popup menu with RMB -> thread dump. Another option is to click Thread Dump button in Threads tab of application open. This should open new tab with textual thread dump, pretty similar to jstack one. 

Also the only thing the tread dumps have shown to me is that its waiting on RefernceQueue.remove, which holds the Finalizer objects that gets created due to finalize methods.

This means that ReferenceQueue is empty. If you wish to dig deeper you need to check same thing with unpatched driver.

Another option is to load OOM heap dump with visual vm, then click "Show threads" in the Summary, then check for Finalizer thread. If it could not keep up, you should see exact object that caused problems.

--
Best regards,
 Vitalii Tymchyshyn