Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

convert exchange API to use async-service #1466

Conversation

@pipermerriam
Copy link
Member

pipermerriam commented Jan 9, 2020

What was wrong?

The p2p.exchange APIs use the old service model.

While fixing this I also encountered a race condition bug in the ResponseCandidateStream logic which is fixed here as well.

How was it fixed?

Updated them to use the new async-service APIS

The ResponseCandidateStream registers the _handle_msg as a handler which is called by the Connection anytime the proper message type is received. The previous implementation used a Future to communicate potential response candidates back to the coroutine which is waiting for a response.

That listening couroutine runs in a loop, Repeatedly calling _get_payload to retrieve a response candidate. After retrieving the result of the pending future, _get_payload creates a new future for subsequent messages to be placed.

The architecture above only works if calls to _handle_msg and _get_payload are evenly interspersed, always being called one after the other. This refactor surfaced a bug where _handle_msg is called in quick succession before _get_payload can re-stock the future, resulting in us dropping what could be the right response due to the future already having a result set on it.

This was fixed by replacing the Future with a Queue. This way if multiple responses arrive in quick succession, they end up being placed into the queue and consumed correctly.

Cute Animal Picture

Pets-Wearing-Christmas-Costumes

@pipermerriam pipermerriam requested a review from carver Jan 9, 2020
@carver
carver approved these changes Jan 10, 2020
Copy link
Contributor

carver left a comment

Yup, I think this all looks fine, but this code has surprised me before. Have you synced with it a bit to make sure nothing too crazy is happening?

response_timeout: float

pending_request: Optional[Tuple[float, 'asyncio.Future[TResponseCommand]']]
pending_request: Optional[Tuple[float, 'asyncio.Queue[TResponseCommand]']]

This comment has been minimized.

Copy link
@carver

carver Jan 10, 2020

Contributor

I'm not sure this should be part of the public API at all, actually.

@@ -33,19 +34,19 @@

class ResponseCandidateStream(
ResponseCandidateStreamAPI[TRequestCommand, TResponseCommand],
BaseService):
Service):
logger = logging.getLogger('p2p.exchange.ResponseCandidateStream')

This comment has been minimized.

Copy link
@carver

carver Jan 10, 2020

Contributor

Should we use the extended debug logger?

This comment has been minimized.

Copy link
@pipermerriam

pipermerriam Jan 10, 2020

Author Member

I've only been using it when we actually use the DEBUG2 log level (which we don't here)

@pipermerriam

This comment has been minimized.

Copy link
Member Author

pipermerriam commented Jan 10, 2020

This has been tested and does indeed function properly syncing blocks, doing another more extended test before merging (tomorrow).

@pipermerriam pipermerriam merged commit 7a426d6 into ethereum:master Jan 10, 2020
44 checks passed
44 checks passed
ci/circleci: docker-trinity-beacon-image-build-test Your tests passed on CircleCI!
Details
ci/circleci: docker-trinity-image-build-test Your tests passed on CircleCI!
Details
ci/circleci: py36-docs Your tests passed on CircleCI!
Details
ci/circleci: py36-eth1-components Your tests passed on CircleCI!
Details
ci/circleci: py36-eth1-core Your tests passed on CircleCI!
Details
ci/circleci: py36-eth1-monitor-trio Your tests passed on CircleCI!
Details
ci/circleci: py36-eth2-core Your tests passed on CircleCI!
Details
ci/circleci: py36-eth2-fixtures Your tests passed on CircleCI!
Details
ci/circleci: py36-eth2-integration Your tests passed on CircleCI!
Details
ci/circleci: py36-eth2-utils Your tests passed on CircleCI!
Details
ci/circleci: py36-integration Your tests passed on CircleCI!
Details
ci/circleci: py36-lightchain_integration Your tests passed on CircleCI!
Details
ci/circleci: py36-lint Your tests passed on CircleCI!
Details
ci/circleci: py36-lint-eth2 Your tests passed on CircleCI!
Details
ci/circleci: py36-long_run_integration Your tests passed on CircleCI!
Details
ci/circleci: py36-p2p Your tests passed on CircleCI!
Details
ci/circleci: py36-p2p-trio Your tests passed on CircleCI!
Details
ci/circleci: py36-rpc-blockchain Your tests passed on CircleCI!
Details
ci/circleci: py36-rpc-state-byzantium Your tests passed on CircleCI!
Details
ci/circleci: py36-rpc-state-constantinople Your tests passed on CircleCI!
Details
ci/circleci: py36-rpc-state-frontier Your tests passed on CircleCI!
Details
ci/circleci: py36-rpc-state-homestead Your tests passed on CircleCI!
Details
ci/circleci: py36-rpc-state-petersburg Your tests passed on CircleCI!
Details
ci/circleci: py36-rpc-state-spurious_dragon Your tests passed on CircleCI!
Details
ci/circleci: py36-rpc-state-tangerine_whistle Your tests passed on CircleCI!
Details
ci/circleci: py36-wheel-cli Your tests passed on CircleCI!
Details
ci/circleci: py37-eth1-components Your tests passed on CircleCI!
Details
ci/circleci: py37-eth1-core Your tests passed on CircleCI!
Details
ci/circleci: py37-eth1-monitor-trio Your tests passed on CircleCI!
Details
ci/circleci: py37-eth2-components Your tests passed on CircleCI!
Details
ci/circleci: py37-eth2-core Your tests passed on CircleCI!
Details
ci/circleci: py37-eth2-fixtures Your tests passed on CircleCI!
Details
ci/circleci: py37-eth2-integration Your tests passed on CircleCI!
Details
ci/circleci: py37-eth2-utils Your tests passed on CircleCI!
Details
ci/circleci: py37-libp2p Your tests passed on CircleCI!
Details
ci/circleci: py37-lint Your tests passed on CircleCI!
Details
ci/circleci: py37-lint-eth2 Your tests passed on CircleCI!
Details
ci/circleci: py37-p2p Your tests passed on CircleCI!
Details
ci/circleci: py37-p2p-trio Your tests passed on CircleCI!
Details
ci/circleci: py37-rpc-state-quadratic Your tests passed on CircleCI!
Details
ci/circleci: py37-rpc-state-sstore Your tests passed on CircleCI!
Details
ci/circleci: py37-rpc-state-zero_knowledge Your tests passed on CircleCI!
Details
ci/circleci: py37-wheel-cli Your tests passed on CircleCI!
Details
continuous-documentation/read-the-docs Read the Docs build succeeded!
Details
@pipermerriam pipermerriam deleted the pipermerriam:piper/convert-exchange-api-to-use-async-service branch Jan 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.