Handshake is setting DialTimeout instead of context deadline

[NamanMahor]

Summary

If a user provides a context.Context with a timeout, it is ignored here in conn_handshake.go even though comment is correct to set context level deadline override any read deadline but instead of deadline we are setting Dialtimeout.

Made change to handle context deadline as we are already doing in here in conn_ping.go

[CLAassistant]

All committers have signed the CLA.

[kavirajk]

@NamanMahor thanks for the PR :). I know it's a simple change. can you please add tests for this to avoid any regression in the future?

[NamanMahor]

@kavirajk sure will add the test. I have one question which could also be bug. we are overriding the ctx with timeout/deadline here https://github.com/ClickHouse/clickhouse-go/blob/main/clickhouse.go#L304 which is being used by ch.dial(ctx) down in the code and later in ch.dial(ctx) will call handshake with same context so effectively we still using the Dial timeout in handshake even after my PR.

[kavirajk]

have one question which could also be bug. we are overriding the ctx with timeout/deadline here https://github.com/ClickHouse/clickhouse-go/blob/main/clickhouse.go#L304 which is being used by ch.dial(ctx) down in the code and later in ch.dial(ctx) will call handshake with same context so effectively we still using the Dial timeout in handshake even after my PR.

The way I see it, we have ctx on public APIs like Query(ctx), Exec(ctx) which are use-passed context. And when acquiring connection from the pool, we create "new" context by setting DialTimeout and call the dial. The handshake is happened to be inside the dial method. Whatever the timeout you passed via public APIs would still be in effect after dial to be used in real queries.

i'm curious what is your use case here?. You trying to set this timeout more dynamically only on the handsake of the whole dial method? What is blocking you to use just dialTimeout for the whole dial call? including handsake?

[NamanMahor]

@kavirajk sorry about late response totally miss this one. I have added the test.