Guarantee exclusive use of HttpClientConnection.
#273
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
|
Test run, confirming this edge case: |
etan-status
added a commit
to vacp2p/nim-libp2p
that referenced
this pull request
Mar 2, 2024
There are two edge cases that currently trigger `KeyError`:
1. `await newStream.open()` succeeds, but Chronos injects an implicit
suspend point when an `{.async.}` function that suspended returns.
During this implicit suspend point, another task could get scheduled
and close the stream, which removes it from `m.channels` again.
`let channel = m.channels[header.streamId` then gives `KeyError`.
More explanation of implicit suspend point on return after await:
status-im/nim-chronos#273
2. During the `Flush the data` block, `await m.connection.readExactly`
is called which may suspend. If it suspends, same as above, another
task may get scheduled that closes the stream, also resulting in the
`KeyError` below.
The `YamuxError` is subsequently properly handled below.
etan-status
added a commit
to vacp2p/nim-libp2p
that referenced
this pull request
Mar 2, 2024
There are two edge cases that currently trigger `KeyError`:
1. `await newStream.open()` succeeds, but Chronos injects an implicit
suspend point when an `{.async.}` function that suspended returns.
During this implicit suspend point, another task could get scheduled
and close the stream, which removes it from `m.channels` again.
`let channel = m.channels[header.streamId` then gives `KeyError`.
More explanation of implicit suspend point on return after await:
status-im/nim-chronos#273
2. During the `Flush the data` block, `await m.connection.readExactly`
is called which may suspend. If it suspends, same as above, another
task may get scheduled that closes the stream, also resulting in the
`KeyError` below.
The `YamuxError` is subsequently properly handled below.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When calling the HTTP
send/openfunctions,acquireConnectioniscalled to obtain a connection in state
Ready. In the next code block,the connection state is advanced to
RequestHeadersSending. However,returning from chronos
asyncprocs yields control, similar toawait.This means that a connection may be added to the pool in
Readystate,and then a different
acquireConnectioncall may obtain a second ref.Introducing a new
Acquiredstate ensures consistency in this case.No tests added due to this being scheduler dependent; ran manual tests
by adding a
doAssert item.state != HttpClientConnectionState.Acquiredbetween
if not(isNil(conn)):andreturn conn. Eventually, the assertgot 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
asyncproc yields.Even if it's not, this should solve current HTTP issues in nimbus-eth2.