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

Fixup HTTP client timeout #2347

Merged
merged 3 commits into from
Aug 13, 2019
Merged

Fixup HTTP client timeout #2347

merged 3 commits into from
Aug 13, 2019

Conversation

s-ludwig
Copy link
Member

See #2344

@s-ludwig
Copy link
Member Author

Note that the wrong timeout handling in the libasync driver didn't seem to be fixable without making changes in the libasync API, which is currently out of my scope.

@s-ludwig s-ludwig merged commit 3d46cda into master Aug 13, 2019
@s-ludwig s-ludwig deleted the fixup_http_client_timeout branch August 13, 2019 10:04
@Geod24
Copy link
Contributor

Geod24 commented Aug 13, 2019

Thanks! Do you have plans for a release any time soon ?

@s-ludwig
Copy link
Member Author

In fact I wanted to have one out weeks ago, but always get distracted by internal stuff. I would try to make a quick pass over the list of open PRs to see if anything is easy to merge and can then tag an RC.

@denizzzka
Copy link
Contributor

I would try to make a quick pass over the list of open PRs to see if anything is easy to merge and can then tag an RC.

#2346

@AndrejMitrovic
Copy link
Contributor

@s-ludwig isn't the timeout supposed to be used with the call to resolveHost too? Here:

https://github.com/bpfkorea/vibe.d/blob/35079d4cf348e36785783480535ff186f2e4ef02/http/vibe/http/client.d#L703-L706

resolveHost itself doesn't take any timeout parameters - and it calls asyncAwaitAny without a timeout (https://github.com/vibe-d/vibe-core/blob/b417214e325a7fb684817b4ae7b218eec46c11c1/source/vibe/core/net.d#L69).

We're having an issue right now where a call to resolveHost blocks. I think maybe resolveHost should also get a timeout parameter and pass it on to asyncAwaitAny.

What do you think?

@s-ludwig
Copy link
Member Author

@AndrejMitrovic Fully agreed. Cancellation is supported by the eventcore API, too, so there is really no reason not do do that.

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.

5 participants