fix(conn): ensure ReadTimeout applies to each conn.Read#1612
fix(conn): ensure ReadTimeout applies to each conn.Read#1612GeorgeMac wants to merge 9 commits intoClickHouse:mainfrom
ReadTimeout applies to each conn.Read#1612Conversation
GeorgeMac
changed the title
ReadTimeout applies to each conn.Read and not just the first blockReadTimeout applies to each conn.Read
GeorgeMac
force-pushed
the
gm/read-timeout
branch
from
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.
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.
|
Thank you for reporting and submitting this, this is the correct fix. However, the test server is A LOT of code to be adding for this one patch. It's a lot of code that will need to be maintained, and I would prefer to find a way to test this through the real ClickHouse server. I fear long term that something will break with the test server and then we have to update both, or vice versa. Ultimately what we need is a Considering the fix is incredibly simple, I decided to just merge the related solution, and we can create an issue to improve testing on this later. We already have some code that tests the read timeout setting and context deadline, so it's not like this is a completely untested feature. Thank you for the solution and detailed comments + PR! I will get this released asap. |
2 participants
Summary
Fixes #1607
This PR addresses the issue of
ReadTimeoutcurrently only applying to the first block returned on a query to ClickHouse.For the Changelog:
The first change in this PR introduces
lib/protoupdates 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: