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

refactor(p2p): better pool closing #2051

Merged
merged 1 commit into from
Jan 12, 2021
Merged

refactor(p2p): better pool closing #2051

merged 1 commit into from
Jan 12, 2021

Conversation

sangaman
Copy link
Collaborator

@sangaman sangaman commented Jan 5, 2021

This makes several improvements to the way we close the p2p pool.

  1. Make the peer close function synchronous by not awaiting the disconnection reason packet being sent. Previously we would wait and hold up the closing flow, which could introduce delays on stale or dead sockets. It also meant there was different behavior for closing peers with or without a disconnection reason. Instead we continue with all the steps for closing a peer and destroy the socket whenever the socket write attempt is complete.

  2. Immediately destroy the socket if it is in connecting state, don't even attempt to send a disconnection reason packet since it
    would not work anyway.

  3. Don't increment the connection failure attempt counter for peers when/after we close the pool and thus are canceling any ongoing connection attempts. This goes against the spirit of the connection failure counter and also means that he pool may be making db writes after it is closed (and potentially after the database has closed).

Then in the Pool test suite, this flips the order of the db close and the pool close so that the pool is second, which makes sense
since it depends on an open, functioning database. It also moves the logic in the before block up to the describe block, the before block wasn't properly completing before the test cases began running. See https://mochajs.org/#asynchronous-code.

This fixes the p2p pool test flakiness issue described here: #1771 (comment).

@sangaman sangaman added p2p Peer to peer networking code quality Improving code structure, organization, and clarity automated tests labels Jan 5, 2021
@sangaman sangaman requested review from a user and raladev January 5, 2021 06:52
@sangaman sangaman self-assigned this Jan 5, 2021
This makes several improvements to the way we close the p2p pool.

1. Make the peer `close` function synchronous by not awaiting the
disconnection reason packet being sent. Previously we would wait and
hold up the closing flow, which could introduce delays on stale or
dead sockets. It also meant there was different behavior for closing
peers with or without a disconnection reason. Instead we continue
with all the steps for closing a peer and destroy the socket whenever
the socket write attempt is complete.

2. Immediately destroy the socket if it is in `connecting` state,
don't even attempt to send a disconnection reason packet since it
would not work anyway.

3. Don't increment the connection failure attempt counter for peers
when/after we close the pool and thus are canceling any ongoing
connection attempts. This goes against the spirit of the connection
failure counter and also means that he pool may be making db writes
after it is closed (and potentially after the database has closed).

Then in the Pool test suite, this flips the order of the db close
and the pool close so that the pool is second, which makes sense
since it depends on an open, functioning database. It also moves
the logic in the `before` block up to the `describe` block, the
`before` block wasn't properly completing before the test cases
began running. See https://mochajs.org/#asynchronous-code.
@sangaman sangaman force-pushed the refactor/pool-closing branch from d53e15d to 3b45d73 Compare January 5, 2021 06:54
@raladev raladev merged commit 9f7673a into master Jan 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automated tests code quality Improving code structure, organization, and clarity p2p Peer to peer networking
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants