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, 16 Jan 2006 17:25:52 GMT
On 1/15/06, Bryan Pendleton <bpendleton@amberpoint.com> wrote:
> Deepa Remesh (JIRA) wrote:
> > I have attached a partial patch 'derby-210-patch1.diff' for this problem.
>
> Hello Deepa,
>
> I was interested to read your patch because I have been trying to learn how
> the client code works.

Thanks Bryan for looking at my patch. This is my first change in the
client code. And your comments are very helpful and have pointed some
more areas which need to be worked on

>  After reading your patch, I had a few small questions:
>
> 1) I was a bit surprised that there didn't seem to be any obvious Java thread
> synchronization protecting the CommitAndRollbackListeners and openStatements
> hash maps. Is there some higher-level synchronization which protects the
> access to these objects?

There is some method-level synchronization (in Connection class) which
provides some synchronization when accessing openStatements list. But
this does not cover all accesses. I did not think about this when I
replaced the LinkedList with WeakHashMap. I understand both these
collection classes are not synchronized and I have not created a any
regression when I replaced with WeakHashMap. This can be something
which needs to be worked on separately, right?

> 2) It seemed a bit unfortunate that you had to touch files like Lob.java and
> PreparedStatement.java. It seemed like a bit of an encapsulation hole to
> allow these other classes to directly access the hash maps in the Connection
> object, rather than going through some sort of convenience method like:
>
>     connection_.addCommandAndRollbackListener(this);


As you point out, it would be better to have convenience methods to
add/remove objects.

Another thing which I have been thinking is whether we need to add
PreparedStatements and Lobs to CommitAndRollbackListeners.

In case of Lobs, I cannot see anything else being done with it other
than adding and removing from this list. Is there any other purpose
for adding Lobs to this list?

For PreparedStatements, the comment mentions they are added to the
list because prepared statements need to be re-prepared after
commit/rollback. As I understand, this is not required. Also, in the
code, only internal statements like statements used for auto-generated
keys etc are being re-prepared. Maybe, this can be avoided.  Again, I
will open a jira issue for this.

>
> 3) I thought it was interesting that Connection.java favors the style:
>
>     final java.util.WeakHashMap openStatements_ = new java.util.WeakHashMap();
>
> as opposed to the more common (I think) style of:
>
>     import java.util.WeakHashMap;
>
>     final WeakHashMap openStatements_ = new WeakHashMap();
>
> Is there a reason that the client code seems to favor the use of explicitly
> qualified class names rather than using import statements?

When I used 'java.util.WeakHashMap', I was just replacing
'java.util.LinkedList'. I was following the general style in the
client code.

I noticed in the client driver that it uses the same class names as
JDBC classes (Connection, Statement, ResultSet etc). So the JDBC
classes are always fully qualified (java.sql.Connection etc). Maybe
this is the reason for using  explicitly qualified class names and
this was done for all classes. I am just guessing here.

>
> Anyway, thanks very much for posting your patch for review. I find it very
> educational to read the patches and try to understand how they work.

Thanks again for looking at my patch. I will open jira issues for all
the issues/improvements.

I have the combined patch for derby-210 ready and will upload it to
jira in a while. It includes the changes I already mentioned plus a
test based on the attached repro. Please take a look and share your
thoughts.

Thanks,
Deepa

Mime
View raw message