fix: Actually close unclosed results. Previously was not closing the first unclosed result fixes #1903

[davecramer]

No description provided.

[codecov-commenter]

Codecov Report

Merging #1905 into master will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##             master    #1905      +/-   ##
============================================
+ Coverage     69.17%   69.19%   +0.01%     
- Complexity     4210     4213       +3     
============================================
  Files           197      197              
  Lines         18005    18006       +1     
  Branches       2919     2920       +1     
============================================
+ Hits          12455    12459       +4     
+ Misses         4204     4202       -2     
+ Partials       1346     1345       -1     

[vlsi]

Should this be placed in closeUnclosedResults?
Otherwise, it sounds strange that closeUnclosedResults leaves an unclosed resultset behind.

[davecramer]

probably yes, one more Q should we include this in 42.2.19 ?

[vlsi]

Let's include it provided it passes tests.

[davecramer]

@vlsi so currently the testExecuteTwice test is failing due to the statement being closed. This seems to be the correct behaviour since ResultSet docs say A ResultSet object is automatically closed when the Statement object that generated it is closed, re-executed, or used to retrieve the next result from a sequence of multiple results. and the test has s.closeOnCompletion(); set and this states that the statement will be closed when the resultsets are closed. This will introduce a change in behaviour. I'm sort of fine with this, but we have been caught before. Thoughts ?

[jorsol]

WDYT about this tests:

  @Test
  public void testCloseOnCompletionMultipleExecutionResultSets() throws SQLException {
    Statement statement = conn.createStatement();
    assertFalse(statement.isCloseOnCompletion());
    ResultSet rs1 = statement.executeQuery("SELECT 1");
    assertFalse("rs1 should be open", rs1.isClosed());
    // a call to closeOnCompletion does effect both the subsequent execution of statements,
    // and statements that currently have open, dependent, result sets.
    statement.closeOnCompletion();
    assertTrue(statement.isCloseOnCompletion());
    ResultSet rs2 = statement.executeQuery("SELECT 2");
    assertTrue("rs1 should be closed by rs2", rs1.isClosed());
    assertFalse("rs2 should be open", rs2.isClosed());
    assertFalse("statement should be open", statement.isClosed());
    rs2.close(); // Affects the statement closeOnCompletion.
    assertTrue("statement should be closed", statement.isClosed());
  }

  @Test
  public void testMultipleExecutionResultSets() throws SQLException {
    Statement statement = conn.createStatement();
    ResultSet rs1 = statement.executeQuery("SELECT 1");
    assertFalse("rs1 should be open", rs1.isClosed());
    ResultSet rs2 = statement.executeQuery("SELECT 2");
    assertTrue("rs1 should be closed by rs2", rs1.isClosed());
    assertFalse("rs2 should be open", rs2.isClosed());
    statement.close(); // closing the statement affects the result set
    assertTrue("statement should be closed", statement.isClosed());
    assertTrue("rs2 should be closed", rs2.isClosed());
  }

[davecramer]

Sure, can't have too many tests,

Except

   statement.closeOnCompletion();
    assertTrue(statement.isCloseOnCompletion());
    ResultSet rs2 = statement.executeQuery("SELECT 2");

Doesn't pass as setting closeOnCompletion will close the existing statement when it closes the existing result set.

[jorsol]

Doesn't pass as setting closeOnCompletion will close the existing statement when it closes the existing result set.

That's what worries me, I'm not completely sure that it should close the statement.
https://docs.oracle.com/javase/8/docs/api/java/sql/Statement.html#closeOnCompletion--

However, a call to closeOnCompletion does effect both the subsequent execution of statements, and statements that currently have open, dependent, result sets.

So it should allow subsequent execution of statements, just that it close the previous Result Set, and when all the Result Sets are closed then the closeOnCompletion should close the statement.

I can't confirm that this is the correct behavior, so we should be careful if we introduce this change of behavior and is not correct.

It's really sad that the JDBC spec is really ambiguous, but if we can ask for a second opinion it would be great.

[davecramer]

Welcome to JDBC

However, a call to closeOnCompletion does effect both the subsequent execution of statements, and statements that currently have open, dependent, result sets.

So it should allow subsequent execution of statements, just that it close the previous Result Set, and when all the Result Sets are closed then the closeOnCompletion should close the statement.

I actually read this as and statements that currently have open, dependent, result sets. as it effect current statements.

I'll see if I can get an answer. Problem is it may not be quick.

[davecramer]

OK, I have searched all of the JSR's and there is no mention however reading the javadocs again.

Specifies that this Statement will be closed when all its dependent result sets are closed. If execution of the Statement does not produce any result sets, this method has no effect.
Note: Multiple calls to closeOnCompletion do not toggle the effect on this Statement. However, a call to closeOnCompletion does effect both the subsequent execution of statements, and statements that currently have open, dependent, result sets.

Note that the first line says "all it's dependent result sets" and the note specifically refers to dependent result sets.

I'm now OK with this change in it's current form

[davecramer]

I sent the following to the jdbc-spec-discuss list:

If we have an open statement with open resultsets and call
closeOnCompletion and then execute the statement a second time the
statement will throw an already closed exception since the spec says
that
executing a statement a second time closes any open resultsets. Closing
said resultsets ends up closing the statement.

I'm just confirming that this is the intended behaviour. Seems about
right
since if you set closeOnCompletion you would not expect to re-use the
statement.

and the response was

Logically, yes, I think this is how it is supposed to work. If you call
closeOnCompletion, you signal that you intended to abandon the statement
after processing, so if you then go and try to use the statement any
way, you're doing something wrong.

So I think what we have is fine.

The bigger question is that since it changes behaviour slightly do we want to include it in 42.2.19

[jorsol]

So I think what we have is fine.

The bigger question is that since it changes behaviour slightly do we want to include it in 42.2.19

It only affects closeOnCompletion(), so I expect that it would not cause any harm in 42.2.19, I think the benefit of closing the resultset when the statement is closed outweighs the drawback of a potential compatibility issue with current code, sooner or later this change should be included to improve the jdbc-spec compatibility anyway, so I'm fine to include it in 42.2.19.

[sehrope]

I took a peek to see what we do now and what other drivers do in this situation. Here's the result for this sample code:

Statement stmt = conn.createStatement();
stmt.closeOnCompletion();
ResultSet rsOne = stmt.executeQuery("SELECT 1");
ResultSet rsTwo = stmt.executeQuery("SELECT 2");
rsTwo.next();

• Postgres v42.2.18 - Allows second execution
• MariaDB v2.7.2 - Allows second execution
• MSSQL v9.2.0 - NPE (!)
• Oracle v19.7.0.0 - SQL Exception for "Closed Statement"

So MariaDB matches our old behavior and Oracle's driver matches up with the proposed new behavior.

[davecramer]

So it seems the JDBC spec group is aligned with this interpretation.