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 issue #1529 : DoTimeout timeout failure #1535

Merged
merged 1 commit into from
Apr 15, 2023

Conversation

kukayiyi
Copy link
Contributor

As discussed in #1529 , the main problem with the timeout failure is that req.timeout is not passed to dial to pause the dialing in time. One solution is to create a temporary dial. In order not to affect the original dial of HostClient, I choose to add the dialTimeout parameter in the dialHostHard function to control the dialing time. It will fail if HostClient.dial has been set, but general users should not set dial. In fact, I think the most appropriate solution is to make HostClient have a Dialer with a timeout attribute so that timeout can be set separately without affecting other settings and use HostClient.Dialer.dial instead of HostClient.dial, but obviously this will change a lot of code.

@kukayiyi
Copy link
Contributor Author

I've looked at other code, and when the connection is successfully established, the timeout is affected by readTimeout and writeTimeout as expected, so this change doesn't change the timeout control normally, only when the connection fails. I think the connection establish time should be able to be set, just like python request

@erikdubbelboer erikdubbelboer merged commit 87cb886 into valyala:master Apr 15, 2023
@erikdubbelboer
Copy link
Collaborator

I agree changing the Dail on HostClient is probably not a good idea as it isn't backwards compatible. If users set their own Dial then they can handle the timeouts in there as well.

I like the change this way. 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.

2 participants