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.
This commit is contained in:
Etan Kissling 2022-05-13 01:54:34 +02:00 committed by GitHub
parent ae19d5b6f0
commit dedd7cb1d0
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
1 changed files with 4 additions and 0 deletions

View File

@ -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