Skip to content
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.

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

fix(query): always close client.conn in cancelQuery (issue #405) #409

Merged
merged 1 commit into from
Jul 1, 2024

Conversation

charredlot
Copy link
Contributor

@charredlot charredlot commented Jun 24, 2024

When the server closes the connection unexpectedly, the client will call cancelQuery (e.g. when client.packet fails). In client.cancelQuery, if client.flushBuf has data to flush, it will return a non-nil error and return early without calling conn.Close. This prevents the chpool from removing the client after client.Do and leaves the client.conn in a bad state such that future writes will always fail with a "broken pipe" error.

Summary

Checklist

Delete items not relevant to your PR:

  • Unit and integration tests covering the common scenarios were added
  • A human-readable description of the changes was provided to include in CHANGELOG

@CLAassistant
Copy link

CLAassistant commented Jun 24, 2024

CLA assistant check
All committers have signed the CLA.

…#405)

When the server closes the connection unexpectedly, the client
will call cancelQuery (e.g. when client.packet fails). In
client.cancelQuery, if client.flushBuf has data to flush, it will
return a non-nil error and return early without calling conn.Close.
This prevents the chpool from removing the client after client.Do
and leaves the client.conn in a bad state such that future writes will
always fail with a "broken pipe" error.
@ernado ernado merged commit f3dc1fe into ClickHouse:main Jul 1, 2024
23 checks passed
@cwegener
Copy link

Interesting find. I am also seeing these error conditions when using the database/sql interface in clickhouse-go@v2.17.1

I do wonder though what is causing the unexpectedly closed connection in the server.

In my case, the TCP connection is via localhost. And my clickhouse server log says 'Broken pipe':

<Error> TCPHandler: Code: 210. DB::NetException: I/O error: Broken pipe, while writing to socket (127.0.0.1:9000 -> 127.0.0.1:41866). (NETWORK_ERROR),

@charredlot
Copy link
Contributor Author

I do wonder though what is causing the unexpectedly closed connection in the server.

In my case, the TCP connection is via localhost. And my clickhouse server log says 'Broken pipe':

In our case, there was an https proxy restarting, so we expected disconnects. This PR doesn't change anything with respect to whether disconnects happen. I'll throw a random guess that maybe one of the conns in your pool hit an idle timeout? But you might get more traction with a reliable repro on the linked issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants