-
Notifications
You must be signed in to change notification settings - Fork 594
fix(conn): ensure ReadTimeout
applies to each conn.Read
#1612
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
base: main
Are you sure you want to change the base?
Conversation
ReadTimeout
applies to each conn.Read
and not just the first blockReadTimeout
applies to each conn.Read
bca33c4
to
27f711c
Compare
ctx, cancel := context.WithTimeout(context.Background(), time.Second) | ||
}) | ||
t.Run("query which returns in time", func(t *testing.T) { | ||
ctx, cancel := context.WithTimeout(context.Background(), 3*time.Second) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note for the reviewer:
This particular context driven deadline is now being checked on each read down in the net stack (it is not being removed after first read) this test became unstable. I tweaked the timeout running the tests with -count 10
until I could get something consistent. Three seconds seemed enough to process all of the response.
Summary
Fixes #1607
This PR addresses the issue of
ReadTimeout
currently only applying to the first block returned on a query to ClickHouse.For the Changelog:
The first change in this PR introduces
lib/proto
updates to support some of the missing client encoding and server decoding functions for CH packets.This was so that I could implement a fake CH server where we can define our own handlers:
https://github.com/GeorgeMac/clickhouse-go/blob/37404af3d6637594d79bf3345a20970ddba01c31/lib/testing/server.go#L57-L85
Then I used this fake server in a couple unit tests. The first a simple happy path
SELECT 1
, mostly to ensure that the fake server was working as expected alongside the client.The second is the case which demonstrates the problem in the issue.
When the test suite is run with a timeout before the fix in 299fe0c:
Which nicely demonstrates in the panic output where the client blocks on
rows.Next()
:Checklist
Delete items not relevant to your PR: