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

@s-ludwig s-ludwig commented Aug 11, 2019

See #2344

@s-ludwig
Copy link
Member Author

@s-ludwig s-ludwig commented Aug 11, 2019

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 force-pushed the fixup_http_client_timeout branch from 3603f26 to 375b802 Compare Aug 11, 2019
@s-ludwig s-ludwig merged commit 3d46cda into master Aug 13, 2019
3 checks passed
@s-ludwig s-ludwig deleted the fixup_http_client_timeout branch Aug 13, 2019
@Geod24
Copy link
Contributor

@Geod24 Geod24 commented Aug 13, 2019

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

@s-ludwig
Copy link
Member Author

@s-ludwig s-ludwig commented Aug 13, 2019

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

@denizzzka denizzzka commented Aug 13, 2019

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

@AndrejMitrovic AndrejMitrovic commented Aug 18, 2020

@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

@s-ludwig s-ludwig commented Aug 18, 2020

@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.

None yet

5 participants