JDBC Client: fix a bug where TrinoResultSet cannot be properly closed

[xiacongling]

ResultSet cannot be properly closed since the inner Thread cannot be interrupted and stop data row iteration. That will lead to thread and memory leaks on the client side. This patch uses FutureTask, which is created by ThreadPoolExecutor, instead of CompletableFuture to make sure InterruptedException can be raised as expected. And for the case that interruption is not properly handled by the underlying StatementClient, a status check is added to the loop condition so that loop can terminate and thread can be released.

Description

Is this change a fix, improvement, new feature, refactoring, or other?

a fix

Is this a change to the core query engine, a connector, client library, or the SPI interfaces? (be specific)

JDBC client

How would you describe this change to a non-technical end user or system administrator?

fix a bug where TrinoResultSet cannot be properly closed which will lead to thread and memory leaks on the client side.

Related issues, pull requests, and links

• Fixes In Trino-JDBC calling TrinoResultSet.cancel can't really close completablaFuture thread #7374
• Fixes An increasing number of WAITING state threads in JDBC driver #14176
• Related to FutureTask replaces CompletableFuture to solve the thread can't be cancelled #7404

Documentation

(x) No documentation is needed.
( ) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.

Release notes

( ) No release notes entries required.
(x) Release notes entries required with the following suggested text:

# JDBC Client
* Fix a bug where TrinoResultSet cannot be properly closed. ({issue}`7374`)

[lhofhansl]

Looks good.
Now I wonder why we have used a CompletableFuture in the first place.

[findepi]

We already have TestJdbcResultSet to test the ResultSet implementation

[xiacongling]

Yes, I notice that. But TestJdbcResultSet is a integration test with test trino server set up. The test cases here are all unit tests. Is it better to keep them separate?

[findepi]

Add a javadoc explaining what it is and also @see tag -- in both these tests.

like

/**
 * A unit test for {@link TrinoResultSet}.
 * 
 * @see TestJdbcResultSet an integration test.
 */

[findepi]

@electrum please take a look

[findepi]

Does the order of these actions (client.close, draining) matter?

[xiacongling]

It doesn't matter for most cases. I am just worried that client may fill rowQueue after it has been drained.

[findepi]

So it may matter. Let's document the reasoning as a code comment

[findepi]

Add a javadoc explaining what it is and also @see tag -- in both these tests.

like

/**
 * A unit test for {@link TrinoResultSet}.
 * 
 * @see TestJdbcResultSet an integration test.
 */

[Twilightuse]

ResultSet cannot be properly closed since the inner Thread cannot be interrupted and stop data row iteration. That will lead to thread and memory leaks on the client side. This patch uses FutureTask, which is created by ThreadPoolExecutor, instead of CompletableFuture to make sure InterruptedException can be raised as expected. And for the case that interruption is not properly handled by the underlying StatementClient, a status check is added to the loop condition so that loop can terminate and thread can be released.

Description

Is this change a fix, improvement, new feature, refactoring, or other?

a fix

Is this a change to the core query engine, a connector, client library, or the SPI interfaces? (be specific)

JDBC client

How would you describe this change to a non-technical end user or system administrator?

fix a bug where TrinoResultSet cannot be properly closed which will lead to thread and memory leaks on the client side.

Related issues, pull requests, and links

• Fixes In Trino-JDBC calling TrinoResultSet.cancel can't really close completablaFuture thread #7374
• Fixes An increasing number of WAITING state threads in JDBC driver #14176
• Related to FutureTask replaces CompletableFuture to solve the thread can't be cancelled #7404

Documentation

(x) No documentation is needed. ( ) Sufficient documentation is included in this PR. ( ) Documentation PR is available with #prnumber. ( ) Documentation issue #issuenumber is filed, and can be handled later.

Release notes

( ) No release notes entries required. (x) Release notes entries required with the following suggested text:

# JDBC Client
* Fix a bug where TrinoResultSet cannot be properly closed. ({issue}`7374`)

trino-jdbc 406 a lot of connection in timed_wating, there is the thread stack
"OkHttp ConnectionPool" #3363 daemon prio=5 os_prio=0 tid=0x00007f403810d800 nid=0x1195 in Object.wait() [0x00007f362a373000]
java.lang.Thread.State: TIMED_WAITING (on object monitor)
at java.lang.Object.wait(Native Method)
at java.lang.Object.wait(Object.java:460)
at io.trino.jdbc.$internal.okhttp3.internal.connection.RealConnectionPool.lambda$new$0(RealConnectionPool.java:62)

locked <0x0000000722d630e0> (a io.trino.jdbc.$internal.okhttp3.internal.connection.RealConnectionPool)
at io.trino.jdbc.$internal.okhttp3.internal.connection.RealConnectionPool$$Lambda$2010/1587346617.run(Unknown Source)
at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
at java.lang.Thread.run(Thread.java:750)
Locked ownable synchronizers:

<0x000000072287d118> (a java.util.concurrent.ThreadPoolExecutor$Worker)
"OkHttp ConnectionPool" #3358 daemon prio=5 os_prio=0 tid=0x00007f3eb0095000 nid=0x118d in Object.wait() [0x00007f362ab7b000]
java.lang.Thread.State: TIMED_WAITING (on object monitor)
at java.lang.Object.wait(Native Method)
at java.lang.Object.wait(Object.java:460)
at io.trino.jdbc.$internal.okhttp3.internal.connection.RealConnectionPool.lambda$new$0(RealConnectionPool.java:62)
locked <0x0000000722ae7c90> (a io.trino.jdbc.$internal.okhttp3.internal.connection.RealConnectionPool)
at io.trino.jdbc.$internal.okhttp3.internal.connection.RealConnectionPool$$Lambda$2010/1587346617.run(Unknown Source)
at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
at java.lang.Thread.run(Thread.java:750)
Locked ownable synchronizers:

<0x000000072261f628> (a java.util.concurrent.ThreadPoolExecutor$Worker)
a