akka-http icon indicating copy to clipboard operation
akka-http copied to clipboard

+htp #1205 Time out requests while they are waiting on the queue

Open jypma opened this issue 7 years ago • 14 comments

Currently, in a situation of a high idle-timeout, a large max-open-requests queue, and a troubled slowly-responding server, the queue is going to fill up. Once the server recovers, potentially very old requests are going to be sent (from much more than idle-timeout ago).

This commit makes no request wait longer than idle-timeout on the queue, since the request WOULD have timed out by then anyways, if a connection had been available.

Fixes #1205.

jypma avatar Sep 28 '17 09:09 jypma

Test PASSed.

akka-ci avatar Sep 28 '17 10:09 akka-ci

@akka/akka-http-team tests green, ready for review.

jypma avatar Sep 28 '17 10:09 jypma

Seems clean enough. I wonder how hard would it be to write a test that tests this. We could increase all other timeouts to make sure that request is timeout out in the queue and not somewhere else.

2m avatar Oct 06 '17 16:10 2m

I decided to re-use the existing timeout mostly for simplicity's sake (understanding client timeouts already is hard enough). We could introduce something like queue-timeout, and update the timeout docs page. But really, this needs a rewrite as well, describing how max-open-requests is actually configuring a queue.

Having a different timeout would of course make it easier to write @2m 's suggested test...

jypma avatar Oct 16 '17 09:10 jypma

I'm working on a test and introduced the separate queueTimeout.

I think I have a problem though: PoolInterfaceActor always gets an initial demand of 16 items from the downstream PoolConductor, irrespective of how many connections are actually available to dispatch requests on. Is there a way to configure that initial demand between the two graph stages?

jypma avatar Oct 25 '17 07:10 jypma

Test FAILed.

akka-ci avatar Nov 21 '17 11:11 akka-ci

PLS BUILD

raboof avatar Dec 04 '17 12:12 raboof

Test PASSed.

akka-ci avatar Dec 04 '17 12:12 akka-ci

Thanks for the extra review! I'm happy to address the remaining comments. However, @jrudolph, weren't you doing some re-write of the client pool stages that would make this irrelevant? Or am I totally making that up...

jypma avatar Dec 05 '17 08:12 jypma

@jrudolph I don't think we expect conflicts here, do we?

raboof avatar Mar 02 '18 09:03 raboof

I should note that this PR does not (fully) work: Because of the 16-item internal akka streams buffer, there will still be 16 items queued up that we can't purge. And though that buffer theoretically can be lowered to 1, I wasn't able to do that accurately without changing the global buffer size.

Plus, going from "we can't purge requests while on the queue" to "we can purge some requests from the queue" may not help with the predictability of the http client... :)

jypma avatar Mar 06 '18 08:03 jypma

Test FAILed.

akka-ci avatar Apr 09 '18 16:04 akka-ci

Test FAILed.

akka-ci avatar Mar 05 '19 11:03 akka-ci

Test FAILed.

!!! Couldn't read commit file !!!

akka-ci avatar Apr 15 '20 10:04 akka-ci