What's the right way currently to set timeouts on aleph client requests?
Probably via manifold.deferred/timeout!. E.g.:
(-> (http/get "")
(d/timeout! 10000)
(d/catch TimeoutException
(log/error "took too long")))
Ah but note that that timeout doesn't include the connection establishment which would be controlled via connection-timeout and defaults to 60s
Which might be surprising depending on whether you consider connection establishment part of the request or not
Hmm does the HTTP client even offer an API for cancelling a request?
Marking the response deferred as failed (via d/error!) doesn't seem to do so
So we can't have a timeout which spans the whole deal with e.g. (-> (http/get "...") (d/timeout! 1000))
it leaves the HTTP request (and connection) hanging
(assuming the connection was successfully established, no request-timeout was given and the other end just doesn't respond any further)
Cancellation is soemthing Zach and Oleksii talked about, but never got around to, sadly
You can see it here: https://github.com/clj-commons/manifold/issues/167
@vale Best bet is one of the :*-timeout params. Arnaud mentioned connection-timeout which measures how long to get a connection.
• connection-timeout
• pool-timeout
• request-timeout
• read-timeout
request-timeout measures time til the initial Ring map is ready, not necessarily including the body. read-timeout measures how long for the overall response to be finished (successfully or not). pool-timeoutis almost the same thing as connection-timeout, I'd ignore it for now
All of those timeouts will result in the overall conn being closed early
(Sorry I misunderstood. A top-level timeout! is still fine if you don't care about killing the connection early.)
the reason I asked about if the timeout on the deferred actually killed the http request is because if you squint at it hard enough it is the same issue as https://github.com/clj-commons/manifold/issues/159; deferreds that are realized after a timeout can be seen as making a request to a timeout server, and that request doesn't complete until the timeout expires, and when the timeout expires it realizes the deferred, but you don't want to leave outstanding requests to the "timeout server" hanging around if the deferreds are canceled
I know the various *-timeout params will result in Netty closing the connection channel, which should free up all associated resources.
As for a top-level timeout! I’d have to check. That issue, and its fix, predate me, and I’m not super-familiar with that code off the top of my head.
Gonna be a while before I can take a look though. Is it an active concern or are you just curious?
Not an active concern at all. The issue I linked is closed. I am not really a manifold user, but I have spent a lot of time poking at the timeout code in core.async where it still has a resource "leak" that roughly corresponds to the one fixed in that ticket, and thinking about how to solve that kind of thing using a generic mechanism instead of separately solving it for each kind of thing
Here we go: https://github.com/clj-commons/aleph/issues/712
FWIW, I almost have a patch ready for the second suggested solution
OK somewhat ready: https://github.com/clj-commons/aleph/pull/714 😉
Does that terminate the http request though?
No, it's not going to cancel the HTTP request.
You might have to use request-timeout instead.
Something along those lines :
(http/get "" {:request-timeout 2000})
Thanks for the reference!
@kingmob AFAICT for a top-level timeout! the resources are not freed up on timeout (both empirically and from my reading of the code). The problem seems to be that the response deferred returned by aleph.http/request doesn't "know" about the inner cleanup-conn callback. Maybe we could attach an error callback with on-realized to it which would dispose of the connection on error if needed?
Want to open a new GH Discussion for the topic? Perhaps it's time to revisit it