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

add support for custom dial function with timeouts #1669

Merged

Conversation

avivcarmis
Copy link
Contributor

No description provided.

// Note that if Dial is set instead of DialTimeout, Dial will ignore Request timeout.
// If you want the tcp dial process to account for request timeouts, use DialTimeout instead.
//
// If not set, DialTimeout is used.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we may consider deprecating this one if you feel like it.
I went with a less aggressive approach and just documented the difference between the two.
it may seem a bit confusing in terms of the difference between them though...

@@ -1839,16 +1872,6 @@ func (c *HostClient) dialHostHard(dialTimeout time.Duration) (conn net.Conn, err
n = 1
}

dial := c.Dial
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this block is removed in favor of the new callDialFunc function

@@ -2591,7 +2626,7 @@ func (c *pipelineConnClient) init() {

func (c *pipelineConnClient) worker() error {
tlsConfig := c.cachedTLSConfig()
conn, err := dialAddr(c.Addr, c.Dial, c.DialDualStack, c.IsTLS, tlsConfig, c.WriteTimeout)
conn, err := dialAddr(c.Addr, c.Dial, nil, c.DialDualStack, c.IsTLS, tlsConfig, 0, c.WriteTimeout)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to not overcomplicate the commit, i left PipelineClient as is - meaning although it does implement a DoTimeout function, it does not support passing it to the dial process and thus does not expose a DialWithTimeout param. Adding it could be quite complex due to the async nature of PipelineClient, the dial process might serve several different requests so to support it we might need to further design how exactly we want to act when we have several different timeouts on a single dial call.

If we do want to support it in PipelineClient, we can either address it in this PR or postpone it to later. I will need to understand how to support it here though

Copy link
Collaborator

@erikdubbelboer erikdubbelboer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please fix the linting issues.

@avivcarmis
Copy link
Contributor Author

ugh, my bad.
done.

@erikdubbelboer erikdubbelboer merged commit 8ca7a9c into valyala:master Nov 27, 2023
14 checks passed
@erikdubbelboer
Copy link
Collaborator

Thanks!

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.

None yet

3 participants