Re: non-trivial finalize() on AbstractJdbc2Statement

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

15.02.12 13:11, Imran ???????(??):
> Hi Guys
>
> I dont think I'm referring to the speed or workings of the finalize()
> method available on AbstractJdbc2Statement. As said earlier I agree
> that it should return quickly if statements are closed by clients and
> also having safeguards around it looks nice. But my point is that by
> having a non-trivial finalize() an extra JdbcxStatement object is been
> created and referenced through a Finalizer, which is in a reference
> queue that would be picked up by the Finalizer thread. And a GC
> wouldn't reclaim the memory until the finalizer thread has done its
> work. As we have many queries executing, the main thread keeps filling
> this queue at a much faster rate than the finalizer thread (which is a
> low priority thread by default) would get to it. Under heavy load this
> paves the way for possible OOM issues as we've noticed. Also just to
> check this, we've patched up the driver we have without the finalize()
> method and we've seen much better GC and have not experienced OOM on
> similar load that resulted in OOMs.
>
> The general feeling as I've learned over the years is that finalize()
> should be used cautiously. So I was wondering if its really required
> for the AbstractJdbc2Statement to have a finalize() that only closes
> the statement, which one would expect the client to do anyway.
>
> Cheers
> -- Imran
>
> On Mon, Feb 13, 2012 at 4:49 PM, Kevin Grittner
> <Kevin(dot)Grittner(at)wicourts(dot)gov <mailto:Kevin(dot)Grittner(at)wicourts(dot)gov>> wrote:
>
> ******* ********<tivv00(at)gmail(dot)com <mailto:tivv00(at)gmail(dot)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
>
>