Skip to content

Do not fail awaiting cache transactions when the client closes - #595

Open
inakisoriamrf wants to merge 1 commit into
ContentSquare:masterfrom
inakisoriamrf:fix/client-close-completes-cache-transaction
Open

inakisoriamrf wants to merge 1 commit into
ContentSquare:masterfrom
inakisoriamrf:fix/client-close-completes-cache-transaction

Conversation

@inakisoriamrf

Copy link
Copy Markdown

Description

When a cacheable query is in flight, identical queries do not run: they await the cache transaction of the first query (AwaitForConcurrentTransaction, up to grace_time).

If the client of the first query closes the connection before the response is complete, chproxy kills the query (KILL QUERY) and completeTransaction marks the transaction as failed. Every query that awaits it then gets HTTP 500 with the body [concurrent query failed] and does not run, although the query itself did not fail. A client that leaves early (a closed browser tab, a cancelled request) makes the identical requests of other clients fail.

This change marks the transaction as completed when the query was cancelled because its client closed the connection. The awaiting queries find no cached result and run the query themselves, as they already do after a 503 or 408 from ClickHouse. A cancelled response is still not cached. Timeouts (context.DeadlineExceeded) and ClickHouse errors still fail the transaction, so the awaiting queries do not repeat a query that would fail again.

Changes:

  • scope.clientClosed is set in proxyRequest when the request ends with context.Canceled. In this path the context starts from context.Background(), the configured query timeout uses context.WithTimeout and ends with context.DeadlineExceeded, and the only other cancellation comes from listenToCloseNotify. So context.Canceled can only mean that the client closed the connection.
  • completeTransaction completes the transaction when s.clientClosed is true.

Pull request type

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe):

Checklist

  • Linter passes correctly (go vet ./... and golangci-lint run v1.64.8: no issues)
  • Add tests which fail without the change (if possible)
  • All tests passing (go test -race ./...)
  • Extended the README / documentation, if necessary

New test TestReverseProxy_ClientCloseCompletesCacheTransaction, with a cache that has a grace time:

  • Query that awaits the cancelled one: query A runs on the server. The identical query B is sent and awaits the transaction of A: the test checks that B did not end and did not reach the server. Then the client of A closes the connection. Without the change, B gets 500 "[concurrent query failed] \n". With the change, B runs the query and gets 200.
  • Same query after the cancelled one: the client of A closes the connection, and the same query is sent at once, while the state of the transaction is still in the registry. Without the change, it gets the same 500. With the change, it gets 200.

The test waits for explicit states (the request in flight on the fake server, B not finished and not on the server) instead of fixed delays only. It passed 10 times in a row with -race, and both cases fail without the change.

Does this introduce a breaking change?

  • Yes
  • No

Further comments

  • After a cancelled query, the queries that await it all find a completed transaction and no cached result, so they all run the query. This is the same behaviour as after a 503 or 408 today. Making one of them take over the transaction is a larger change and is out of scope here.

  • An alternative is to leave the transaction failed and let clients retry on [concurrent query failed]. That moves the problem to every client, and the error message does not tell a cancelled query from a failed one.

  • A related limit that this change does not address: a request that waits in the user queue (max_queue_time) or in AwaitForConcurrentTransaction does not watch its own client. A client that closes during the wait still holds its slot until the wait ends.

When a cacheable query is in flight, identical queries await its cache
transaction. If the client of the first query closed the connection,
the query was killed and the transaction was marked as failed. The
awaiting queries then got HTTP 500 "[concurrent query failed]" and did
not run, although nothing was wrong with the query.

Mark the transaction as completed when the query was cancelled because
its client closed the connection. The awaiting queries find no cached
result and run the query themselves. Timeouts and ClickHouse errors
still fail the transaction.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

1 participant