Adds HTTP client timeout setting #2344
Conversation
denizzzka
force-pushed
the
client_timeout
branch
from
f1f94ef
to
cb10817
Compare
http/vibe/http/client.d
Outdated
| @@ -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 | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Ok, sure
It just cost me a bit of time and I decided to pay attention to this fact. I’ll delete it now.
|
@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? |
|
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
force-pushed
the
client_timeout
branch
from
02952f3
to
44b496a
Compare
|
CI tests passed, with the exception of 3 pcs due to network problems on the CI server side |
|
Still one win32 test failed due to CI side. Can it be restarted? |
|
Yes. just tried to do so, but it looks like only the whole job can be restarted :/ |
|
It should be okay to simply document that only vibe-core currently supports connection timeouts and then use BTW, what exactly is the intent of this change, to define the |
|
@wilzbach They have finally added a |
|
Please restart one failed test if you can? |
|
@denizzzka: Even though this has already been merged, can you clarify my last question? This doesn't seem to do what the API states. |
|
The intent is to define a read timeout, so that it can be used with |
|
Okay, then I'll rename it to "read timeout" to make that clear. The initial implementation that passed the timeout to |
I do not agree with this proposal, it complicates. My thoughts: 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.
Yes, at first I also thought of doing the first, but the second was implemented only. That's right, you can rename it. |
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? |
|
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). |
Fixes #1375