hibernate-devel

Re: [Hibernate-devel] not so fast! :)

[Gavin_King/Cirrus%<CI...@ci...>]

> Thanks for the quick response on 0.9.11!  Except it makes me feel
> a bit ungrateful to have to report another bug now - hopefully
> this'll be my last one for a while.

I sure hope its the last, if only to save _me_ further embarrasment!
I've applied your suggested fix to CVS, and also fixed the deprecated
versions. At some stage along the way I stopped testing hibernate in
the supply-your-own connection scenario - a bad decision which has
opened us up to these bugs. As a high priority, I need to write some
tests for this functionality.

> BTW, this problem might seem fairly benign - just a log warning,
> as long as your connections don't mind being closed twice.  But
> in theory, it would be possible for finalize() to trigger after
> an externally-pooled connection has been reactivated, and thus
> close an active connection.  That might have been
> fun to track down!  ;)

Urgghhh. Thanks for picking it up at this point.

I guess this means I have to do another release real soon ... but
not today. I might do a next release after I integrate Christoph
Beck's datatype path - which was my next task.

Many Thanks

Gavin.

RE: [Hibernate-devel] not so fast! :)

[Gavin_King/Cirrus%<CI...@ci...>]

> Hey, these would only be embarrassing if they were caught after
> v1.0!

Yeah, but I'm kinda hoping 1.0 is fairly close now.....

> I figured the external connection scenario wasn't being widely
> used.  I probably wouldn't be using it either, except that I'm
> integrating into an existing system, and I thought it would be
> a good idea to share the same connection pool as the rest of
> the system.

Exactly the kind of usage I had in mind :)

> > I guess this means I have to do another release real soon ... but
> > not today.

> I agree it's not that urgent, unless you're aware of anyone
> embedding the code into heart-lung machines as we speak... :)

heh :) heart-lung machine programmers can use the version in CVS

> I was thinking about that: testing the finalize issue would be
> tricky, since the symptoms could range from benign to unpredictable.
> I think this particular issue would be handled better by some
> additional checks (or assertions).  One would be inside
> RelationalDatabaseSession.finalize:

Okay, I integrated that...

> Another place to check for and warn about a non-null connection
> would be inside cleanup(), i.e.

probably not necessary - that stuffs better done in the tests
(as you noted).

Unfortunately the various possible failure modes make this section
of the code (ie. Session.close() + the deprecated versions) very
fiddly and difficult to fully test.

> My first bug, with the parameters swapped around, is easier to unit
> test for.  I'll see if I can put something together after I've
> installed the code I'm currently working on.  (Unit test after
> release, XLP - Xtremely Lazy Programming?)

:)

Anton, if you send me your sourceforge username, I will give you CVS
access.

[Hibernate-devel] SortedSet & SortedMap bugfix (?)

[Anton v. S. <an...@ap...>]

I was crashing trying to load a SortedSet, with "java.lang.RuntimeException:
Hibernate failed trying to load a collection".  The log showed that a
ClassCastException was occurring in java.util.TreeMap.compare.  It turned
out that this was because the TreeMap in question didn't have a comparator,
although I had specified one.  I traced this to line 24 of SortedSet.java
(in CVS & v0.9.11):

	java.util.SortedSet clonedSet = new TreeSet();

This looks to me as though it should read:

	java.util.SortedSet clonedSet = new TreeSet(comparator);

...which fixes the problem for me.  There's an equivalent line in SortedMap
which I expect suffers from the same problem.

This problem presumably only occurs if the objects being compared don't
support the Comparable interface - if they do, TreeMap.compare uses that.

I won't make this my first change in CVS, since I haven't attempted to
understand the SortedSet logic in any detail, and for all I know this change
could affect something else.

Anton

RE: [Hibernate-devel] not so fast! :)

[Anton v. S. <an...@ap...>]

Hey, these would only be embarrassing if they were caught after v1.0!  To
Hibernate's credit, this latest one at least gave a log warning.  I figured
the external connection scenario wasn't being widely used.  I probably
wouldn't be using it either, except that I'm integrating into an existing
system, and I thought it would be a good idea to share the same connection
pool as the rest of the system.

> I guess this means I have to do another release real soon ... but
> not today.

I agree it's not that urgent, unless you're aware of anyone embedding the
code into heart-lung machines as we speak... :)

> As a high priority, I need to write some
> tests for this functionality.

I was thinking about that: testing the finalize issue would be tricky, since
the symptoms could range from benign to unpredictable.  I think this
particular issue would be handled better by some additional checks (or
assertions).  One would be inside RelationalDatabaseSession.finalize:

	if (conn!=null) {
		log.warn("Unclosed session");
		if (conn.isClosed())
			log.warn("Inconsistent state: unclosed session with closed connection");
		else
			conn.close();
	}

The second warning triggers in my case - that's how I verified what was
going on.

Another place to check for and warn about a non-null connection would be
inside cleanup(), i.e.

	if (conn!=null) log.warn("Unclosed connection");

I believe this would also have triggered in my case, although I didn't try
it.  I guess you could also exercise this kind of thing with some white-box
unit tests.

My first bug, with the parameters swapped around, is easier to unit test
for.  I'll see if I can put something together after I've installed the code
I'm currently working on.  (Unit test after release, XLP - Xtremely Lazy
Programming?)

Anton