Fix client-v2: cancelTransportRequest lost between two retry attempts - #2990
Open
polyglotAI-bot wants to merge 3 commits into
Open
Fix client-v2: cancelTransportRequest lost between two retry attempts#2990polyglotAI-bot wants to merge 3 commits into
polyglotAI-bot wants to merge 3 commits into
Conversation
…ween retry attempts
Cancellation state lived on the per-attempt TransportRequest, so a
cancelTransportRequest(queryId) landing between two attempts of a retried
operation was silently dropped: on the query path the next attempt overwrote the
registry entry with a fresh un-cancelled request, and on both insert paths the
entry was unregistered after every attempt, so the cancel was a complete no-op.
The retry guard then saw an un-cancelled (or missing) request and the operation
retried and could succeed.
Cancellation is now tracked per operation: the registry holds an OngoingOperation
with a sticky cancelled flag, registration lives for the whole operation on all
three retry loops, a request attached to an already cancelled operation is
cancelled right away, and every retry iteration re-checks the flag before issuing
another request. A cancelled operation now fails with the same
TransportException("Request was cancelled on client side") that an in-flight
cancellation produces.
Fixes: #2989
Client V2 CoverageCoverage Report
Class Coverage
|
JDBC V2 CoverageCoverage Report
Class Coverage
|
JDBC V1 CoverageCoverage Report
Class Coverage
|
Client V1 CoverageCoverage Report
Class Coverage
|
…, not when its first request is created
With useAsyncRequests(true) the operation body runs on the shared executor, so an
operation registered itself in the cancellation registry only once the executor
picked it up. A cancelTransportRequest(queryId) issued right after the call
returned found no entry, was silently dropped, and the operation then ran to
completion - the front edge of the same window that was closed between attempts.
Operations are now registered on the calling thread, before being submitted, and
the cancellation guard runs before every attempt (including the first), so a
cancelled operation fails with TransportException("Request was cancelled on
client side") without sending a request. A registration is dropped again when the
submission is rejected, and the registry is cleared on close() because an
operation still queued when the executor is shut down never runs.
Fixes: #2989
Collaborator
Author
|
Added a follow-up commit closing the front edge of the same cancellation window. With CompletableFuture<QueryResponse> f = client.query(sql, settingsWithQueryId);
client.cancelTransportRequest(queryId); // could land before the supplier registeredChanges in that commit:
Test:
|
…ry and insert retry loops The retry decision after a failed attempt was written three times, once per retried operation (POJO insert, stream insert, query), so the cancellation fix of this PR had to touch all three copies - which SonarCloud reported as duplication on new code. The decision is now taken by one private helper: it rethrows the wrapped failure when no further request may be issued (a non-retryable failure, or an operation cancelled on the client side) and otherwise returns the endpoint of the next attempt. Behaviour is unchanged: the same exception instance is rethrown, and on the last attempt the node is still rotated without changing the endpoint. To keep the helper's parameter list short, an operation now carries the query id and the settings its attempts run with, and is therefore always created - it is only put into the cancellation registry when it has a query id to be addressed by.
Collaborator
Author
|
Pushed The duplicated lines were the retry decision after a failed attempt, which existed three times in
Behaviour is unchanged — same exception instance rethrown, and on the last attempt the node is still rotated without changing the endpoint (as before, where the returned endpoint was discarded). To keep the helper's parameter list short, an operation now carries its query id and the settings its attempts run with; it is always created and only put into the cancellation registry when it has a query id to be addressed by. Verified in a devbox against a live 25.8 server: |
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fixes #2989.
Cancellation state lived on the per-attempt
TransportRequest(ConcurrentHashMap<String, TransportRequest> ongoingRequests, keyed by query id), while a retried operation creates a new request per attempt. AClient.cancelTransportRequest(queryId)that landed in the window between two attempts was therefore silently dropped: on the query path the next attempt'sregisterTransportReqoverwrote the entry with a fresh, un-cancelled request; on both insert paths the entry was unregistered at the end of every attempt, socancelTransportRequestwas a complete no-op (getreturnednull). Either way therequestIsNotCancelled(...)retry guard returnedtrue, so the operation issued another request and could complete successfully — the caller's cancellation was lost. On the stream-insert path that window is reachable deterministically through public API, because the client callsDataStreamWriter#onRetry()exactly there.The fix moves cancellation from the request to the operation: the registry holds an
OngoingOperationwith a stickycancelledflag, its lifetime is the whole operation (all three retry loops), a request attached to an already-cancelled operation is cancelled right away, and every retry iteration re-checks the flag before creating the next request. A cancelled operation fails with the sameTransportException("Request was cancelled on client side")that an in-flight cancellation produces (the last attempt's failure is kept as cause), so a caller sees one outcome regardless of when the cancellation landed. The cancellation state does not outlive the operation: the entry is removed in an outerfinally, and only the operation's own entry is removed (remove(key, value)), so a concurrent operation reusing the same query id is unaffected.Not covered here (adjacent, separate window): a cancellation issued before the operation has registered itself — e.g. immediately after submitting an async operation — is still a no-op, matching the documented "only that still waiting for response" wording.
Changes
client-v2/.../api/Client.javaongoingRequestsnow maps a query id to a new privateOngoingOperationholder (stickycancelledflag + currently attached request;attach()/cancel()synchronized).registerOperation(queryId)/unregisterOperation(queryId, operation)replace the per-attempt register/unregister of the raw request;registerTransportReq(operation, request)only attaches the current attempt's request, and the retry guard reads the operation's flag.try/finallyand the stream-insert per-attemptunregisterTransportReqis dropped, so the registration lives for the whole operation on all three paths (the query path already did).cancelledException(lastException, queryId)at the top of a retry iteration when the operation was cancelled, instead of sending another request.cancelTransportRequestjavadoc states that cancellation applies to the whole operation.docs/features.md: the client-side cancellation entry now states that cancellation applies to the whole operation, including between attempts.CHANGELOG.md: bug-fix entry with the issue link.Test
client-v2/.../api/transport/TransportBaseTests.java—testCancelBetweenRetryAttempts(TestNG@DataProvider, two rows), next to the existing cancel/retry tests and reusing their WireMock helpers. The mocked server returns a retryable503(X-ClickHouse-Exception-Code: 202) on the first request and200afterwards; the cancel is issued fromDataStreamWriter#onRetry(), i.e. deterministically between the two attempts.cancelled: the operation must fail as cancelled (TransportException) and the mock must observe exactly one request. Onmainthe insert completes successfully and the mock records two requests, so the test fails there for the right reason. The same query id is then reused by a second insert that must succeed, pinning that the sticky flag does not outlive the operation.other-query-id(contrast): cancelling an unrelated query id must leave the operation alone — it recovers on the retry with exactly two requests, so the new guard does not blanket-stop retries.Runs (devbox, ClickHouse
latest):TransportBaseTests24/24 green;client-v2unit tests green;InsertTests+QueryTests126/126 green.HttpTransportTestshas 27 failures both with and without this patch (pre-existing environment failures: SSL client cert / named test users).docs/changes_checklist.mdtestRetriesAndSucceedsAfterRetryableServerErrorfor all three paths). No defaults, config keys or per-request overrides change. Retry logging is untouched: the stop happens before the next attempt, so no extra retry WARN is emitted. Documented behavior is updated indocs/features.md.TransportException,isRetryable = false), so cancellation stays classified as non-retryable. Paths that are not cancelled keep throwing exactly what they threw before.i > 0(a retry), so the first attempt is unchanged; anullquery id yields anulloperation handle and the guard is a no-op, exactly asrequestIsNotCancelled(null)was before.lastExceptionis always assigned before an iteration can continue, so it is nevernullat the guard.Pre-PR validation gate
main, passes with the fix, same command)mainAGENTS.md/docs/changes_checklist.md/docs/features.mdCHANGELOG.mdupdatedClient#insertentry point end-to-end)