Skip to content

Conversation

@takezoe
Copy link
Member

@takezoe takezoe commented Dec 11, 2024

Description

Currently, StatementClient.close() doesn't send a cancel request after hitting the client-side timeout because state is changed here:

if (sinceStart.compareTo(requestTimeoutNanos) > 0) {
state.compareAndSet(State.RUNNING, State.CLIENT_ERROR);
throw new RuntimeException(format("Error fetching next (attempts: %s, duration: %s)", attempts, sinceStart), cause);
}

while close() sends a cancel request only when state is RUNNING:

public void close()
{
// If the query is not done, abort the query.
if (state.compareAndSet(State.RUNNING, State.CLIENT_ABORTED)) {
URI uri = currentResults.get().getNextUri();
if (uri != null) {
httpDelete(uri);
}
}
}

Since close() is called before changing state when the thread is interrupted, it looks like applying the same way is natural solution:

catch (InterruptedException e) {
try {
close();
}
finally {
Thread.currentThread().interrupt();
}
state.compareAndSet(State.RUNNING, State.CLIENT_ERROR);
throw new RuntimeException("StatementClient thread was interrupted");
}

However, I have one concern that state is changed to CLIENT_ABORTED in close() in this case, so state.compareAndSet(State.RUNNING, State.CLIENT_ERROR); doesn't seem to take effect in the code above. I'm not sure how it should work in this case.

Additional context and related issues

Release notes

( ) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
( ) Release notes are required, with the following suggested text:

## Section
* Fix some things. ({issue}`issuenumber`)

@cla-bot cla-bot bot added the cla-signed label Dec 11, 2024
@github-actions
Copy link

github-actions bot commented Jan 1, 2025

This pull request has gone a while without any activity. Tagging for triage help: @mosabua

@github-actions github-actions bot added the stale label Jan 1, 2025
@hashhar hashhar requested a review from electrum January 10, 2025 16:58
@github-actions github-actions bot removed the stale label Jan 10, 2025
@wendigo wendigo self-requested a review January 10, 2025 18:18
@mosabua
Copy link
Member

mosabua commented Jan 28, 2025

Reminder @wendigo and @electrum

@wendigo wendigo force-pushed the close-client-when-timeout branch from e0090b0 to 641f574 Compare January 29, 2025 10:38
@wendigo wendigo merged commit 4971ef1 into trinodb:master Jan 29, 2025
94 checks passed
@github-actions github-actions bot added this to the 470 milestone Jan 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

3 participants