#7 Extent.closeAll() bug fixes

closed
Kelly Grizzle
None
5
2003-01-08
2002-12-19
J. David Beutel
No

I found and fixed two bugs with Extent.closeAll():

1. It throws a ClassCastException when it has open
iterators.

2. After a close, the iterators throw a JDOException
from hasNext() and next(), but the 1.0 spec requires
them to return false and throw NoSuchElementException
respectively. This bug and fix also applies to
Extent.close(), Query.close(), and Query.closeAll().

diff -r -u
tjdo_cvs_2002dec09/src/com/triactive/jdo/store/ClassBaseTableExtent.java
tjdo_cvs_2002dec09patched/src/com/triactive/jdo/store/ClassBaseTableExtent.java
--- tjdo_cvs_2002dec09/src/com/triactive/jdo/store/ClassBaseTableExtent.java
Fri Nov 8 14:06:24 2002
+++ tjdo_cvs_2002dec09patched/src/com/triactive/jdo/store/ClassBaseTableExtent.java Thu Dec 19 12:27:56 2002
@@ -181,7 +181,7 @@

while (i.hasNext())
{
- QueryResult qr = (QueryResult)i.next();
+ QueryResult qr =
(QueryResult)queryResultsByIterator.get( i.next() );

qr.close();
i.remove();
diff -r -u
tjdo_cvs_2002dec09/src/com/triactive/jdo/store/QueryResult.java
tjdo_cvs_2002dec09patched/src/com/triactive/jdo/store/QueryResult.java
--- tjdo_cvs_2002dec09/src/com/triactive/jdo/store/QueryResult.java Fri Oct 18 06:00:57 2002
+++ tjdo_cvs_2002dec09patched/src/com/triactive/jdo/store/QueryResult.java
Thu Dec 19 13:00:40 2002
@@ -52,9 +52,14 @@
nextResultSetElement();
}

+ private boolean isOpen()
+ {
+ return resultObjs != null;
+ }
+
private void assertIsOpen()
{
- if (resultObjs == null)
+ if( ! isOpen() )
throw new JDOUserException("Query result
has been closed");
}

@@ -98,7 +103,8 @@
{
synchronized (QueryResult.this)
{
- assertIsOpen();
+ if( ! isOpen() )
+
return false;

return nextRowNum < resultObjs.size()
? true : moreResultSetRows;
}
@@ -108,7 +114,8 @@
{
synchronized (QueryResult.this)
{
- assertIsOpen();
+ if( ! isOpen() )
+ throw new
NoSuchElementException("This query result has been
closed");

Object nextElement = null;

I also modified build.xml to easily run a single test
case from the command line, like so:

$ ant unit-tests-firebird -Dtestcase=ExtentTest

The results still go to the log file in the
test_results directory.

diff -r -u tjdo_cvs_2002dec09/build.xml
tjdo_cvs_2002dec09patched/build.xml
--- tjdo_cvs_2002dec09/build.xml Fri Dec 13 08:57:41 2002
+++ tjdo_cvs_2002dec09patched/build.xml Thu Dec 19 11:52:14 2002
@@ -300,7 +300,10 @@

<classpath refid="project.testclasspath"/>

<formatter type="xml"/>

<batchtest todir="${project.testresults}">
-
<fileset dir="${project.src}"
includes="**/test/**/*Test.java"/>
+
<fileset dir="${project.src}">
+
<include name="**/test/**/*Test.java" unless="testcase"/>
+
<include name="**/test/**/${testcase}.java" if="testcase"/>
+
</fileset>

</batchtest>

</junit>
</target>

I also wrote one unit test which demonstrates both bugs
and fixes. I'll attach the unit test and the three
files I changed. I tested this fix with all the unit
tests, but only on Firebird (and ant 1.5.1, java 1.4.1,
RedHat 7.2).

By the way, the TJDO code looks good! I only looked at
a little bit of it, but it was pretty easy to
understand by itself. On the other hand, since I'm not
familiar with the TJDO code, please integrate this
patch cautiously.

Thanks,
11011011

Discussion

  • unit test to demonstrate both bugs and fixes

     
    Attachments
  • patched source file

     
  • patched source file

     
    Attachments
  • patched build file

     
    Attachments
  • Logged In: YES
    user_id=484527

    Oops, well, I didn't notice Mathew Cooper's patch for the
    same bug a month ago. I thought this felt familiar! I
    guess I should have checked the bug database first. But:

    1. His patch is marked as closed, but it is not in the CVS log.

    2. He fixed it by iterating on the value collection instead
    of the keySet. That seems like a viable alternative. I
    didn't do it that way because I wasn't sure that a remove
    from the value collection would remove the entry from the
    HashMap, but when I saw Mathew's patch I checked the javadoc
    and it looks okay.

     
  • Logged In: YES
    user_id=484527

    I also noticed that Multithreaded mode is not (yet)
    supported. Is it required?

    I'm just curious---is there a design for its support? I
    assume the purpose is to manage a trade-off between speed
    and concurrency/synchronization.

    But, if it's required, then the simplest solution might be
    to have no trade-off and always run in multithreaded mode
    (i.e., make everything thread-safe).

    11011011

     
  • Logged In: YES
    user_id=484527

    So, ClassBaseTableExtent.java is redundant with Mathew
    Cooper's patch, but the other three source files are still
    valid. Please consider integrating them.

    Thanks,
    11011011

     
  • Kelly Grizzle
    Kelly Grizzle
    2003-01-08

    • assigned_to: nobody --> pierreg0
    • status: open --> pending
     
  • Kelly Grizzle
    Kelly Grizzle
    2003-01-08

    Logged In: YES
    user_id=524390

    Thanks for the patch. I checked it in as-is, except for
    ClassBaseTableExtent, which I used from patch 639849.

    As far as your multithreaded question, I'm not sure of the
    answer. Try posting it to one of the forums to start a
    discussion.

    --Kelly

     
  • Kelly Grizzle
    Kelly Grizzle
    2003-01-08

    • status: pending --> closed