db-derby-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Deepa Remesh <drem...@gmail.com>
Subject Re: [jira] Updated: (DERBY-210) Network Server will leak prepared statements if not explicitly closed by the user until the connection is closed
Date Mon, 13 Feb 2006 17:35:46 GMT
On 2/12/06, Bryan Pendleton <bpendleton@amberpoint.com> wrote:
> Deepa Remesh (JIRA) wrote:
> > I am attaching a draft patch 'derby-210-v2-draft.diff' for review.
> Hi Deepa,
> I studied this patch for a while, focusing specifically on your bullet
> points 8, 9, and 10. The changes look good to me, and I think you are
> definitely on the right track.

Thanks Bryan for reviewing the patch.

> I had just a few ideas/requests that I wanted to pass along:
> - If you're still looking for ways to break the big patch into smaller
>    ones, change #10 seems to me like it should be separable.

I was planning to submit 8,9,10 bullets in my description as one
patch. But it seems better to separate 10 as a separate patch. Then I
plan to submit 1,3,4 as one patch. And after that the patch to enable
the test derbyStress.java to run with client driver.

> - Regarding change #10, it seems like "needsToSendParamData" is reset
>    to false by rsClose(), which is called by close(). So maybe you don't
>    need to reset it to false again in close(). In some ways, it
>    seems like a simpler way to code change #10 would be to add
>         currentDrdaRs = new DRDAResultSet();
>    to DRDAStatement.rsClose(), rather than doing it in DRDAStatement.close().
>    But I think the way you've done it is fine, too.

I have removed reset of "needsToSendParamData". I have kept reset of
currentDrdaRs in
DRDAStatement.close() itself since close() method resets variables in
DRDAStatement. I was trying to see if I can avoid doing a "new
DRDAResultSet()" in the close method since it looks a little odd to
create a new object in close method. I guess that is fine since
close() method is also called when re-using the statement.

> - In changes #8 and #9, it seems like it would be more consistent to name
>    the new PreparedStatement method "markClosed" rather than "cleanupParameters".
>    Since this markClosed() method would override the one in Statement, you'd
>    need to be sure to call super.markClosed() during the method. It seems like,
>    in addition to being more consistent with the superclass, this would also
>    arrange for the new cleanup code to be called in a few more cases, such as
>    when methods in the Connection class call Statement.markClosed().

I have renamed the method to markClosed(). As you pointed out,
cleanupParameters is the equivalent of markClosed.

> - Lastly, it seems like one of the big parts of your change is to identify
>    a few simple rules about which methods are safe to call from the finalizers,
>    and which are not; namely, that finalizer-triggered code is purely local
>    whereas close code may send messages to the server to call close code there.
>    I think it would be good to add some comments to the Statement.java and
>    PreparedStatement.java classes to record this analysis, so that future
>    readers understand the distinction between the close() and markClosed()
>    methods.

I have added more comments. Please go through them and let me know if
I have missed something. I will attach the patches to JIRA in a while.


View raw message