Copilot commented on code in PR #17381:
URL: https://github.com/apache/iotdb/pull/17381#discussion_r3003664696
##########
iotdb-client/session/src/main/java/org/apache/iotdb/session/pool/SessionPool.java:
##########
@@ -788,7 +788,7 @@ private void occupy(ISession session) {
occupied.put(session, session);
}
- /** close all connections in the pool */
+/** close all connections in the pool and unblocks any waiting threads*/
Review Comment:
The new Javadoc line is not indented like the surrounding code and is
missing standard Javadoc formatting (capitalization/period and space before
closing `*/`). This may violate style/checkstyle and reads unpolished. Please
format it consistently (indent to match the method, use a proper sentence like
“Closes all connections in the pool and unblocks any waiting threads.”).
```suggestion
/**
* Closes all connections in the pool and unblocks any waiting threads.
*/
```
##########
iotdb-client/session/src/test/java/org/apache/iotdb/session/pool/SessionPoolTest.java:
##########
@@ -1623,4 +1624,71 @@ private List<ByteBuffer> FakedFirstFetchTsBlockResult() {
return Collections.singletonList(tsBlock);
}
+
+ // Regression test for graceful shutdown
+ @Test(timeout = 5000)
+ public void testCloseNotifiesWaitingThreads() throws Exception {
+ SessionPool pool =
+ new SessionPool.Builder()
+ .host("localhost")
+ .port(6667)
+ .user("root")
+ .password("root")
+ .maxSize(1)
+ .waitToGetSessionTimeoutInMs(10000)
+ // notifyAll()
+ .build();
+
+ try {
+ Session mockSession = Mockito.mock(Session.class);
+ ConcurrentLinkedDeque<ISession> queue =
+ (ConcurrentLinkedDeque<ISession>) Whitebox.getInternalState(pool,
"queue");
+ queue.push(mockSession);
+ Whitebox.setInternalState(pool, "size", 1);
+
+ ISession occupiedSession = (ISession) Whitebox.invokeMethod(pool,
"getSession");
+ assertEquals(mockSession, occupiedSession);
+ assertEquals(0, queue.size());
+
+ final Exception[] caughtException = {null};
+
+ Thread waiterThread =
+ new Thread(
+ () -> {
+ try {
+ Whitebox.invokeMethod(pool, "getSession");
+ } catch (Exception e) {
+ caughtException[0] = e;
+ }
+ });
+ waiterThread.start();
+
+ Thread.sleep(100);
+
+ long closeStartTime = System.currentTimeMillis();
+ pool.close();
+ long closeEndTime = System.currentTimeMillis();
+
+ waiterThread.join(1000);
+
+ assertNotNull("Waiter thread should have caught an exception",
caughtException[0]);
+ assertTrue(
+ "Exception should be IoTDBConnectionException",
+ caughtException[0] instanceof IoTDBConnectionException);
+ assertTrue(
+ "Exception message should indicate pool is closed",
+ caughtException[0].getMessage().contains("closed"));
+
+ long closeDuration = closeEndTime - closeStartTime;
+ assertTrue(
+ "close() should complete quickly, but took " + closeDuration + "ms",
+ closeDuration < 1000);
Review Comment:
The `closeStartTime/closeEndTime` measurement here doesn’t assert the key
behavior (that the waiting thread is unblocked quickly); `close()` may be fast
even if the waiter is still blocked. Prefer measuring/limiting the waiter
unblocking time (e.g., time from `close()` to waiter exit) or drop this
duration assertion to avoid slow-CI flakiness.
##########
iotdb-client/session/src/test/java/org/apache/iotdb/session/pool/SessionPoolTest.java:
##########
@@ -1623,4 +1624,71 @@ private List<ByteBuffer> FakedFirstFetchTsBlockResult() {
return Collections.singletonList(tsBlock);
}
+
+ // Regression test for graceful shutdown
+ @Test(timeout = 5000)
+ public void testCloseNotifiesWaitingThreads() throws Exception {
+ SessionPool pool =
+ new SessionPool.Builder()
+ .host("localhost")
+ .port(6667)
+ .user("root")
+ .password("root")
+ .maxSize(1)
+ .waitToGetSessionTimeoutInMs(10000)
+ // notifyAll()
Review Comment:
Minor: the inline `// notifyAll()` comment in the builder chain doesn’t add
information (it doesn’t configure anything) and can be removed to keep the test
clear and intention-revealing.
```suggestion
```
##########
iotdb-client/session/src/main/java/org/apache/iotdb/session/pool/TableSessionPool.java:
##########
@@ -36,6 +36,7 @@ public ITableSession getSession() throws
IoTDBConnectionException {
return sessionPool.getPooledTableSession();
}
+ //Closes the underlying session pool and unblocks any waiting threads.
Review Comment:
The added comment has no space after `//` and is a line comment on a public
method. If the intent is to document behavior for API users, prefer a proper
Javadoc comment (and keep formatting consistent) rather than `//Closes ...`.
```suggestion
/**
* Closes the underlying session pool and unblocks any waiting threads.
*/
```
##########
iotdb-client/session/src/main/java/org/apache/iotdb/session/pool/SessionPool.java:
##########
@@ -819,6 +819,8 @@ public synchronized void close() {
this.closed = true;
queue.clear();
occupied.clear();
+ // Notify all waiting threads in getSession() so they wake up immediately
+ this.notifyAll();
Review Comment:
The PR description says waiting threads could remain blocked for “up to the
full timeout period”, but `getSession()` currently uses `wait(1000)`, so the
pre-fix worst-case delay appears to be ~1s rather than
`waitToGetSessionTimeoutInMs`. Consider updating the description (or the
implementation, if full-timeout blocking is still expected) so the rationale
matches the current code.
##########
iotdb-client/session/src/test/java/org/apache/iotdb/session/pool/SessionPoolTest.java:
##########
@@ -1623,4 +1624,71 @@ private List<ByteBuffer> FakedFirstFetchTsBlockResult() {
return Collections.singletonList(tsBlock);
}
+
+ // Regression test for graceful shutdown
+ @Test(timeout = 5000)
+ public void testCloseNotifiesWaitingThreads() throws Exception {
+ SessionPool pool =
+ new SessionPool.Builder()
+ .host("localhost")
+ .port(6667)
+ .user("root")
+ .password("root")
+ .maxSize(1)
+ .waitToGetSessionTimeoutInMs(10000)
+ // notifyAll()
+ .build();
+
+ try {
+ Session mockSession = Mockito.mock(Session.class);
+ ConcurrentLinkedDeque<ISession> queue =
+ (ConcurrentLinkedDeque<ISession>) Whitebox.getInternalState(pool,
"queue");
+ queue.push(mockSession);
+ Whitebox.setInternalState(pool, "size", 1);
+
+ ISession occupiedSession = (ISession) Whitebox.invokeMethod(pool,
"getSession");
+ assertEquals(mockSession, occupiedSession);
+ assertEquals(0, queue.size());
+
+ final Exception[] caughtException = {null};
+
+ Thread waiterThread =
+ new Thread(
+ () -> {
+ try {
+ Whitebox.invokeMethod(pool, "getSession");
+ } catch (Exception e) {
+ caughtException[0] = e;
+ }
+ });
+ waiterThread.start();
+
+ Thread.sleep(100);
+
+ long closeStartTime = System.currentTimeMillis();
+ pool.close();
+ long closeEndTime = System.currentTimeMillis();
+
+ waiterThread.join(1000);
+
Review Comment:
This regression test is likely flaky and may not reliably prove the
`notifyAll()` behavior. `getSession()` uses `wait(1000)`, and the test joins
for 1000ms; without `notifyAll()`, the waiting thread can still wake on the 1s
timeout and the test may pass. Consider coordinating with a `CountDownLatch` to
ensure the waiter is actually in the wait loop before calling `close()`, and
assert the waiter unblocks well under 1s (e.g., join for a few hundred ms and
assert the thread is no longer alive).
##########
iotdb-client/session/src/test/java/org/apache/iotdb/session/pool/SessionPoolTest.java:
##########
@@ -1623,4 +1624,71 @@ private List<ByteBuffer> FakedFirstFetchTsBlockResult() {
return Collections.singletonList(tsBlock);
}
+
+ // Regression test for graceful shutdown
+ @Test(timeout = 5000)
+ public void testCloseNotifiesWaitingThreads() throws Exception {
+ SessionPool pool =
+ new SessionPool.Builder()
+ .host("localhost")
+ .port(6667)
+ .user("root")
+ .password("root")
+ .maxSize(1)
+ .waitToGetSessionTimeoutInMs(10000)
+ // notifyAll()
+ .build();
+
+ try {
+ Session mockSession = Mockito.mock(Session.class);
+ ConcurrentLinkedDeque<ISession> queue =
+ (ConcurrentLinkedDeque<ISession>) Whitebox.getInternalState(pool,
"queue");
+ queue.push(mockSession);
+ Whitebox.setInternalState(pool, "size", 1);
+
+ ISession occupiedSession = (ISession) Whitebox.invokeMethod(pool,
"getSession");
+ assertEquals(mockSession, occupiedSession);
+ assertEquals(0, queue.size());
+
+ final Exception[] caughtException = {null};
+
+ Thread waiterThread =
+ new Thread(
+ () -> {
+ try {
+ Whitebox.invokeMethod(pool, "getSession");
+ } catch (Exception e) {
+ caughtException[0] = e;
+ }
+ });
+ waiterThread.start();
+
+ Thread.sleep(100);
+
+ long closeStartTime = System.currentTimeMillis();
+ pool.close();
+ long closeEndTime = System.currentTimeMillis();
+
+ waiterThread.join(1000);
+
+ assertNotNull("Waiter thread should have caught an exception",
caughtException[0]);
+ assertTrue(
+ "Exception should be IoTDBConnectionException",
+ caughtException[0] instanceof IoTDBConnectionException);
+ assertTrue(
+ "Exception message should indicate pool is closed",
+ caughtException[0].getMessage().contains("closed"));
+
+ long closeDuration = closeEndTime - closeStartTime;
+ assertTrue(
+ "close() should complete quickly, but took " + closeDuration + "ms",
+ closeDuration < 1000);
+
+ } finally {
+ try {
+ pool.close();
+ } catch (Exception e) {
Review Comment:
The empty catch block in the `finally` silently swallows failures and makes
debugging harder if `close()` starts throwing unexpectedly. At minimum, add a
short `// ignore` comment, or log/`fail()` when an exception is thrown
(depending on the intended behavior).
```suggestion
} catch (Exception e) {
// ignore: best-effort cleanup in test
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
