Skip to content

Commit

Permalink
Guarantee exclusive use of HttpClientConnection. (#273)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
etan-status authored May 12, 2022
1 parent ae19d5b commit dedd7cb
Showing 1 changed file with 4 additions and 0 deletions.
4 changes: 4 additions & 0 deletions chronos/apps/http/httpclient.nim
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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:
Expand All @@ -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]
Expand All @@ -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

Expand Down

0 comments on commit dedd7cb

Please sign in to comment.