Skip to content

Fix SessionPool client leak on reconnection and query failures, and preserve server error messages #1

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.

Sign up for GitHub

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

Merged
CritasWang merged 6 commits into from
Feb 6, 2026
+36 −9
Merged

Fix SessionPool client leak on reconnection and query failures, and preserve server error messages #1

Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
bc96073
Initial plan
Copilot Feb 5, 2026
27cfabd
Merge remote-tracking branch 'upstream/main' into copilot/sync-upstre…
Copilot Feb 5, 2026
acd1f4e
Fix SessionPool client return bug on reconnection failure
Copilot Feb 5, 2026
fb8feb5
Fix client leak when query operations throw exceptions
Copilot Feb 5, 2026
1b3408d
Improve exception handling clarity and reduce duplication
Copilot Feb 5, 2026
2721efd
Preserve server error messages in exception handling
Copilot Feb 5, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 36 additions & 9 deletions src/Apache.IoTDB/SessionPool.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,9 +157,12 @@ protected internal SessionPool(List<string> nodeUrls, string username, string pa
public async Task<TResult> ExecuteClientOperationAsync<TResult>(AsyncOperation<TResult> operation, string errMsg, bool retryOnFailure = true, bool putClientBack = true)
{
Client client = _clients.Take();
bool shouldReturnClient = true;
bool operationSucceeded = false;
try
{
var resp = await operation(client);
operationSucceeded = true;
return resp;
}
catch (TException ex)
Expand All @@ -169,16 +172,25 @@ public async Task<TResult> ExecuteClientOperationAsync<TResult>(AsyncOperation<T
try
{
client = await Reconnect(client);
return await operation(client);
var resp = await operation(client);
operationSucceeded = true;
return resp;
}
catch (TException retryEx)
catch (Exception retryEx)
{
throw new TException(errMsg, retryEx);
// Reconnection failed or retry operation failed
// Client is closed by Reconnect, should not be returned to pool
Comment on lines +181 to +182

Copilot AI Feb 6, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment is misleading. The second line states "Client is closed by Reconnect, should not be returned to pool" but this is only true when Reconnect itself fails. When Reconnect succeeds but the retry operation fails, the client is NOT closed and SHOULD be returned to the pool. This misleading comment is related to the critical bug in this code block.

Copilot uses AI. Check for mistakes.
shouldReturnClient = false;
// Preserve original error message from server
string detailedMsg = $"{errMsg}. {retryEx.Message}";
Comment on lines +184 to +185

Copilot AI Feb 6, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment "Preserve original error message from server" is somewhat inaccurate. The concatenation includes errMsg (a generic context string, not from server) and retryEx.Message (which may or may not be from the server). A more accurate comment would be "Preserve detailed error information" or "Include both context and exception details in error message".

Copilot uses AI. Check for mistakes.
throw new TException(detailedMsg, retryEx);
}
Comment on lines +179 to 187

Copilot AI Feb 6, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Critical bug: This catch block cannot distinguish between reconnection failure and retry operation failure. When Reconnect succeeds (line 174) but the retry operation (line 175) fails, shouldReturnClient is set to false, causing a healthy client to be lost and the pool to shrink. The code should only set shouldReturnClient=false if Reconnect itself throws, not if the retry operation throws. Consider wrapping Reconnect in a separate try-catch or using a flag to track whether a new client was successfully created.

Copilot uses AI. Check for mistakes.
}
else
{
throw new TException(errMsg, ex);
// Preserve original error message from server
string detailedMsg = $"{errMsg}. {ex.Message}";
Comment on lines +191 to +192

Copilot AI Feb 6, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same issue as line 184-185. The comment "Preserve original error message from server" is somewhat inaccurate since the concatenation includes both errMsg (generic context) and ex.Message (which may or may not be from the server).

Copilot uses AI. Check for mistakes.
throw new TException(detailedMsg, ex);
}
}
catch (Exception ex)
Expand All @@ -188,21 +200,36 @@ public async Task<TResult> ExecuteClientOperationAsync<TResult>(AsyncOperation<T
try
{
client = await Reconnect(client);
return await operation(client);
var resp = await operation(client);
operationSucceeded = true;
return resp;
}
catch (TException retryEx)
catch (Exception retryEx)
{
throw new TException(errMsg, retryEx);
// Reconnection failed or retry operation failed
// Client is closed by Reconnect, should not be returned to pool
Comment on lines +209 to +210

Copilot AI Feb 6, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment is misleading. The second line states "Client is closed by Reconnect, should not be returned to pool" but this is only true when Reconnect itself fails. When Reconnect succeeds but the retry operation fails, the client is NOT closed and SHOULD be returned to the pool. This is the same issue as in the first catch block.

Copilot uses AI. Check for mistakes.
shouldReturnClient = false;
// Preserve original error message from server
string detailedMsg = $"{errMsg}. {retryEx.Message}";
Comment on lines +212 to +213

Copilot AI Feb 6, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment inaccuracy as in previous blocks. The comment should reflect that this concatenates context with exception details, not specifically "server error messages".

Copilot uses AI. Check for mistakes.
throw new TException(detailedMsg, retryEx);
}
Comment on lines +207 to 215

Copilot AI Feb 6, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Critical bug: Same issue as lines 179-187. This catch block cannot distinguish between reconnection failure and retry operation failure. When Reconnect succeeds (line 202) but the retry operation (line 203) fails, shouldReturnClient is set to false, causing a healthy client to be lost and the pool to shrink. The code should only set shouldReturnClient=false if Reconnect itself throws, not if the retry operation throws.

Copilot uses AI. Check for mistakes.
}
else
{
throw new TException(errMsg, ex);
// Preserve original error message from server
string detailedMsg = $"{errMsg}. {ex.Message}";
Comment on lines +219 to +220

Copilot AI Feb 6, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment inaccuracy as in previous blocks.

Copilot uses AI. Check for mistakes.
throw new TException(detailedMsg, ex);
}
}
finally
{
if (putClientBack)
// Return client to pool if:
// 1. putClientBack is true (normal operations - client should always be returned), OR
// 2. putClientBack is false (query operations) BUT operation failed, meaning SessionDataSet
// wasn't created and won't manage the client
// Do NOT return if reconnection failed (shouldReturnClient is false) because client was closed by Reconnect
bool shouldReturnForQueryFailure = !putClientBack && !operationSucceeded;
if (shouldReturnClient && (putClientBack || shouldReturnForQueryFailure))
{
_clients.Add(client);
}
Expand Down
Loading