From dedd7cb1d0ac79e63d7481021e069a1d32324060 Mon Sep 17 00:00:00 2001 From: Etan Kissling Date: Fri, 13 May 2022 01:54:34 +0200 Subject: [PATCH] Guarantee exclusive use of `HttpClientConnection`. (#273) When calling the HTTP `send` / `open` functions, `acquireConnection` is called to obtain a connection in state `Ready`. In the next code block, the connection state is advanced to `RequestHeadersSending`. However, returning from chronos `async` procs yields control, similar to `await`. This means that a connection may be added to the pool in `Ready` state, and then a different `acquireConnection` call may obtain a second ref. Introducing a new `Acquired` state ensures consistency in this case. No tests added due to this being scheduler dependent; ran manual tests by adding a `doAssert item.state != HttpClientConnectionState.Acquired` between `if not(isNil(conn)):` and `return conn`. Eventually, the assert got hit after several hours of repeated tests, confirming the edge case to be solved by applying this fix. Not sure if it is by design that returning from an `async` proc yields. Even if it's not, this should solve current HTTP issues in nimbus-eth2. --- chronos/apps/http/httpclient.nim | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/chronos/apps/http/httpclient.nim b/chronos/apps/http/httpclient.nim index b0757c0a..20d9f1b7 100644 --- a/chronos/apps/http/httpclient.nim +++ b/chronos/apps/http/httpclient.nim @@ -38,6 +38,7 @@ type Resolving, ## Resolving remote hostname Connecting, ## Connecting to remote server Ready, ## Connected to remote server + Acquired, ## Connection is acquired for use RequestHeadersSending, ## Sending request headers RequestHeadersSent, ## Request headers has been sent RequestBodySending, ## Sending request body @@ -583,6 +584,7 @@ proc acquireConnection(session: HttpSessionRef, await session.connect(ha).wait(session.connectTimeout) except AsyncTimeoutError: raiseHttpConnectionError("Connection timed out") + res[].state = HttpClientConnectionState.Acquired session.connections.mgetOrPut(ha.id, default).add(res) return res else: @@ -599,6 +601,7 @@ proc acquireConnection(session: HttpSessionRef, else: nil if not(isNil(conn)): + conn[].state = HttpClientConnectionState.Acquired return conn else: var default: seq[HttpClientConnectionRef] @@ -607,6 +610,7 @@ proc acquireConnection(session: HttpSessionRef, await session.connect(ha).wait(session.connectTimeout) except AsyncTimeoutError: raiseHttpConnectionError("Connection timed out") + res[].state = HttpClientConnectionState.Acquired session.connections.mgetOrPut(ha.id, default).add(res) return res