Add idle connection timeouts for HTTP client's connections pool. #324
Conversation
Add timestamps and duration for both HTTP client requests/responses. Add test.
cheatfate
force-pushed
the
http-client-idle-connections
branch
from
9dfb66e to
be77bfb
Compare
Adjust default idle connection timeout to 60 seconds.
| maxConnections = -1): HttpSessionRef {. | ||
| maxConnections = -1, | ||
| idleTimeout = HttpConnectionIdleTimeout, | ||
| idlePeriod = HttpConnectionCheckPeriod): HttpSessionRef {. |
There was a problem hiding this comment.
this is imo an implementation detail that we shouldn't expose in public API - it doesn't matter greatly since we can always make it a no-op, but it feels like the kind of thing that doesn't greatly matter for the user of the API
There was a problem hiding this comment.
I think some of the users could decrease idlePeriod to 1.seconds just to have more precise control.
| @@ -1079,13 +1211,16 @@ proc open*(request: HttpClientRequestRef): Future[HttpBodyWriter] {. | |||
| try: | |||
| let headers = request.prepareRequest() | |||
| request.connection.state = HttpClientConnectionState.RequestHeadersSending | |||
| request.setTimestamp() | |||
There was a problem hiding this comment.
shouldn't this be part of prepareRequest?
There was a problem hiding this comment.
prepareRequest is not related to actual data sending process, so why we need to start from there?
|
|
||
| template isIdle(conn: HttpClientConnectionRef, timestamp: Moment, | ||
| timeout: Duration): bool = | ||
| (timestamp - conn.timestamp) >= timeout |
There was a problem hiding this comment.
could it happen that some really long send (several mb?) causes the connection to be seen as idle?
There was a problem hiding this comment.
Of course its possible, but we can't control it, if people willing to send gigabytes of data from memory to a socket, how we can check if send was actually happened or not?
chronos/apps/http/httpclient.nim
Outdated
| except AsyncTimeoutError: | ||
| raiseHttpConnectionError("Connection timed out") | ||
| conn.state = HttpClientConnectionState.Acquired | ||
| session.connections.mgetOrPut(ha.id, default).add(conn) |
There was a problem hiding this comment.
| session.connections.mgetOrPut(ha.id, default).add(conn) | |
| session.connections.mgetOrPut(ha.id).add(conn) |
There was a problem hiding this comment.
proc mgetOrPut*[A, B](t: var Table[A, B], key: A, val: B): var B =
https://github.com/nim-lang/Nim/blob/v1.6.12/lib/pure/collections/tables.nim#L441
Add timestamps and duration for both HTTP client requests/responses.