Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cursor.close() closes its statement even if the ExecutorType is REUSE #1351

Closed
ryo-murai opened this issue Sep 14, 2018 · 14 comments
Closed
Assignees
Labels
bug no backward compatibility Includes change no backward compatibility for previous version
Milestone

Comments

@ryo-murai
Copy link

ryo-murai commented Sep 14, 2018

DefaultCursor.java L117

Cursor.close() method always closes its statement object regardless the ExecutorType.
In case the ExecuteType is REUSE, it will cause a problem because the Prepared Statement will also be closed and no longer reusable.

MyBatis version

3.4.6

Database vendor and version

postgresql 9.4

Test case or example project

to quickly reproduce the problem, modify some existing tests like below.

// CursorSimpleTest.java
// add below test case
    @Test
    public void shouldGetUser2Times() throws IOException {
        try (SqlSession sqlSession = sqlSessionFactory.openSession()) {
            Mapper mapper = sqlSession.getMapper(Mapper.class);
            Cursor<User> usersCursor = mapper.getAllUsers();
            usersCursor.forEach(System.out::println);

            usersCursor.close();
            Assert.assertFalse(usersCursor.isOpen());

            // 2nd invocation
            usersCursor = mapper.getAllUsers();
            usersCursor.forEach(System.out::println);

            usersCursor.close();
            Assert.assertFalse(usersCursor.isOpen());
        }
    }
<!-- mybatis-config.xml -->
<configuration>
    <settings>
        <setting name="defaultExecutorType" value="REUSE"/>
    </settings>

    <dataSource type="UNPOOLED">
        <property name="driver" value="org.postgresql.Driver" />
        <property name="url" value="jdbc:postgresql://localhost:5432/postgres" />
        <property name="username" value="postgres" />
        <property name="password" value="your_password" />
    </dataSource>
</configuration>

Steps to reproduce

see above test case.

  • run postgresql database on local machine
  • set ExecutorType to REUSE
  • Obtain a Cursor by invoking a Mapper.
  • Close the Cursor.
  • Invoke the Mapper again.

please note that the problem didn't occur on h2db.

Expected result

  • can invoke the Mapper successfully at 2nd and subsequent times.

Actual result

  • an exception occurs in the jdbc driver.
org.postgresql.util.PSQLException: このステートメントは閉じられました。
  • EDIT: I did re-test in en_US locale. Above mentioned is the same error message translated in Japanese.
Caused by: org.postgresql.util.PSQLException: This statement has been closed.
	at org.postgresql.jdbc.PgStatement.checkClosed(PgStatement.java:649)
	at org.postgresql.jdbc.PgStatement.getQueryTimeoutMs(PgStatement.java:520)
	at org.postgresql.jdbc.PgStatement.getQueryTimeout(PgStatement.java:505)
@harawata
Copy link
Member

Thank you, @ryo-murai !
I could reproduce the problem.
Although the behavior might be specific to PostgreSQL driver, it seems to be better to let the executor close statement.
I'm going to remove statement.close() from DefaultCursor.

@gdarmont
Please let me know if there is any concern.

@harawata harawata self-assigned this Sep 24, 2018
@harawata harawata added the bug label Sep 24, 2018
@gdarmont
Copy link
Contributor

Hi,

I did not foresee such usage, so I did not test that case.
After a quick code browsing, I see that when using a ReuseExecutor, it's the doFlushStatements method that will close the statement. In that case, removing statement.close() from DefaultCursor seems ok.
But SimpleExecutor has not such behavior when calling doFlushStatements. So it might leads to resources leak. I may be wrong, it's been a while since I coded these parts :)
Of course, this needs to be tested to check execution paths.

@harawata
Copy link
Member

Hi @gdarmont ,

Thank you for the comment!
Regarding non-reuse executors, invoking Statement#closeOnCompletion() in doQueryCursor() seems to be the simplest way.
It requires JDBC 4.1 compliant drivers, though.

@harawata
Copy link
Member

harawata commented Sep 26, 2018

I did quick tests using a simple JDBC app and found a few drivers that do not support closeOnCompletion().

  • OK : oracle, mysql, mssql, db2, mariadb, postgresql, SAP ASE (since 16 SP04)
  • NG
    • hsqldb, h2 : Closing ResultSet does not close Statement. No exception.
    • Sybase (SAP ASE) (up to 16 SP03): jconn4.jar does not support JDBC 4.1 and it results in an exception ( java.lang.AbstractMethodError: Method com/sybase/jdbc4/jdbc/SybStatement.closeOnCompletion()V is abstract).

It may not be a big problem to hsqldb and h2 users, but it could be to Sybase users.
Does anyone know if there is a newer version of the driver?
It's hard to believe JDBC 4.0 (which is Java 1.6) is the latest supported API for a commercial product.
[UPDATE] There is a new version jconn42.jar released with SAP ASE 16 SP04 and it does support closeOnCompletion(). 👍

@ryo-murai
Copy link
Author

Hi !
Thank you for your time to investigate this issue.

Regarding to the SAP ASE jConnect, it supports JDBC 4.0 specification, according to the online manual site.

And sadly I can't find any newer version other than the 'SAP jConnect for JDBC 16.0' above stated...

IMHO of another way to fix, let Executor tell Cursor its mode or flags so that the Cursor could determine to close or not close its statement... It seems straightforward but I have no idea on how this impacts those design.

@harawata
Copy link
Member

harawata commented Oct 2, 2018

It may be possible to fix this without using closeOnCompletion(), of course, but it will look much uglier. :(

As MyBatis 3.5.0 requires Java 8, it's not so unreasonable to require JDBC 4.1 (=Java 7) compliant driver to use Cursor, I think.
And Sybase users (who need Cursor) can continue to use 3.4.6 until @SAP updates the driver. 😈

@kazuki43zoo ,
What do you think?

@kazuki43zoo
Copy link
Member

@harawata I agree with your opinion.

@harawata
Copy link
Member

harawata commented Oct 3, 2018

Great! I'll work on it.
Thank you all. :)

@harawata harawata added this to the 3.5.0 milestone Oct 6, 2018
@harawata harawata added the no backward compatibility Includes change no backward compatibility for previous version label Oct 6, 2018
@harawata
Copy link
Member

harawata commented Oct 6, 2018

Hi all,

The fix has been committed.
I would suggest to try the latest 3.5.0-SNAPSHOT and see if everything is alright. :)
Thank you!

@arhont375
Copy link

Just to add on this, hive jdbc driver also does not implement this method 😢
https://github.com/apache/hive/blob/master/jdbc/src/java/org/apache/hive/jdbc/HiveStatement.java#L254

@remika
Copy link

remika commented Dec 30, 2019

For information the method closeOnCompletion is not implemented in Apache Phoenix at this time
https://github.com/apache/phoenix/blob/master/phoenix-core/src/main/java/org/apache/phoenix/jdbc/PhoenixStatement.java#L2079

@harawata
Copy link
Member

harawata commented Jan 7, 2020

@arhont375 , @remika
Track these issues, please.
https://issues.apache.org/jira/browse/HIVE-22698
https://issues.apache.org/jira/browse/PHOENIX-5659

@harawata
Copy link
Member

HIVE-22698 has been merged (fix version = 4.0.0). 🎉

@harawata
Copy link
Member

SAP ASE 16 SP04 contains a new driver jconn42.jar which supports closeOnCompletion(), so its users should be able to use Cursor now. 🎉

pulllock pushed a commit to pulllock/mybatis-3 that referenced this issue Oct 19, 2023
- Closing Statement may be an executor's responsibility.
- Using Cursor now requires drivers that support Statement#closeOnCompletion() (JDBC 4.1+).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug no backward compatibility Includes change no backward compatibility for previous version
Projects
None yet
Development

No branches or pull requests

6 participants