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

Adds HTTP client timeout setting #2344

Merged
merged 3 commits into from Aug 10, 2019

Conversation

@denizzzka
Copy link
Contributor

commented Aug 6, 2019

Fixes #1375

@denizzzka denizzzka force-pushed the denizzzka:client_timeout branch from f1f94ef to cb10817 Aug 6, 2019
Copy link
Member

left a comment

Looks good to merge apart from the comment.

@@ -363,6 +380,7 @@ final class HTTPClient {
*/
void connect(string server, ushort port = 80, bool use_tls = false, const(HTTPClientSettings) settings = defaultSettings)
{
//FIXME: actually this function is not connects, but destroys existing connection

This comment has been minimized.

Copy link
@s-ludwig

s-ludwig Aug 6, 2019

Member

This should be removed. Maybe the method name should eventually be changed to setConnectionTarget or something along those lines, but the implementation is working as intended (and it has nothing to do with the other changes). BTW, although it doesn't physically open a TCP connection, "connect" is in line with the BSD socket API when using datagram based sockets, which are in a way similar to a request based protocol like HTTP.

This comment has been minimized.

Copy link
@denizzzka

denizzzka Aug 6, 2019

Author Contributor

Ok, sure

It just cost me a bit of time and I decided to pay attention to this fact. I’ll delete it now.

@denizzzka

This comment has been minimized.

Copy link
Contributor Author

commented Aug 7, 2019

@s-ludwig I am sorry for distracting you. What do you think about the last commit?

Currently, I’m not ready to go deep into fix of the libasync driver itself. I just want to use vibe-core.

Maybe there are better ideas?

@denizzzka

This comment has been minimized.

Copy link
Contributor Author

commented Aug 8, 2019

Wrote by way so that driver users which have timeouts unsupported see that they have such ability - just need to slightly fix this drivers.

@denizzzka denizzzka force-pushed the denizzzka:client_timeout branch from 02952f3 to 44b496a Aug 8, 2019
@denizzzka

This comment has been minimized.

Copy link
Contributor Author

commented Aug 8, 2019

CI tests passed, with the exception of 3 pcs due to network problems on the CI server side

@wilzbach wilzbach added the auto-merge label Aug 9, 2019
@denizzzka

This comment has been minimized.

Copy link
Contributor Author

commented Aug 9, 2019

Still one win32 test failed due to CI side. Can it be restarted?

@wilzbach

This comment has been minimized.

Copy link
Member

commented Aug 9, 2019

Yes. just tried to do so, but it looks like only the whole job can be restarted :/

@s-ludwig

This comment has been minimized.

Copy link
Member

commented Aug 9, 2019

It should be okay to simply document that only vibe-core currently supports connection timeouts and then use version (Have_vibe_core) to handle the tcpConnectionTimeout setting (leaving it as a normal field and possibly emitting a runtime warning if it is not Duration.max).

BTW, what exactly is the intent of this change, to define the connect timeout, a read timeout, or both? I was assuming the first, but currently the second is implemented.

@s-ludwig

This comment has been minimized.

Copy link
Member

commented Aug 9, 2019

@wilzbach They have finally added a re-run incomplete action a while ago (which really is re-run failed)

@dlang-bot dlang-bot removed the auto-merge label Aug 9, 2019
@denizzzka

This comment has been minimized.

Copy link
Contributor Author

commented Aug 10, 2019

Please restart one failed test if you can?

@wilzbach wilzbach added the auto-merge label Aug 10, 2019
@dlang-bot dlang-bot merged commit 08ce218 into vibe-d:master Aug 10, 2019
3 checks passed
3 checks passed
codecov/patch 57.142% of diff hit (target 36.034%)
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@s-ludwig

This comment has been minimized.

Copy link
Member

commented Aug 11, 2019

@denizzzka: Even though this has already been merged, can you clarify my last question? This doesn't seem to do what the API states.

@Geod24

This comment has been minimized.

Copy link
Contributor

commented Aug 11, 2019

The intent is to define a read timeout, so that it can be used with RestInterfaceClient

@s-ludwig

This comment has been minimized.

Copy link
Member

commented Aug 11, 2019

Okay, then I'll rename it to "read timeout" to make that clear.

The initial implementation that passed the timeout to connectTCP was defining a connect timeout and no read timeout, which added to the confusion.

@denizzzka

This comment has been minimized.

Copy link
Contributor Author

commented Aug 11, 2019

It should be okay to simply document that only vibe-core currently supports connection timeouts and then use version (Have_vibe_core) to handle the tcpConnectionTimeout setting (leaving it as a normal field and possibly emitting a runtime warning if it is not Duration.max).

I do not agree with this proposal, it complicates. My thoughts:
Really I do not know how many drivers are implemented in vibe.d, but I know for sure that only two of them are broken and these two are highlighted by version directives.

If fix will be delayed, but someone wants to add a new driver, then it is better to let him immediately see that he still needs to implement timeouts in order to pass the tests.

BTW, what exactly is the intent of this change, to define the connect timeout, a read timeout, or both? I was assuming the first, but currently the second is implemented.

Yes, at first I also thought of doing the first, but the second was implemented only. That's right, you can rename it.

@denizzzka

This comment has been minimized.

Copy link
Contributor Author

commented Aug 11, 2019

The intent is to define a read timeout, so that it can be used with RestInterfaceClient

Could there be necessary timeout when write too? Maybe it is need to implement read+write timeout on sockets?

For example, some kind of stuck with write buffers at busied system can force more and more new connections to be created and wait endlessly for write to it?

@s-ludwig

This comment has been minimized.

Copy link
Member

commented Aug 11, 2019

I've opened a PR that addresses by concerns and also introduces a "connect" timeout: #2347

W.r.t. making the timeout a constant on buggy driver implementation vs. emitting a runtime warning - making it a constant would require application developers to make very unobvious compile time switches if they still want to be able to compile against one of those drivers. This would go against the basic idea of providing a uniform API across platforms. We are also talking about a feature that is usually non-critical, and the runtime warning that gets emitted for every connection should also be very noticable.

Agreed about adding a configurable write timeout. Both, read and write timeouts should also be reviewed in terms of their semantics (does the full operation time out, or the time until some data arrives or gets sent).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.