net: refactor: cleanup ThreadSocketHandler #14335

pull pstratem wants to merge 4 commits into bitcoin:master from pstratem:2018-09-24-thread-handler-cleanup changing 2 files +257 −239

pstratem commented at 9:15 PM on September 26, 2018: contributor

Working towards using poll() on unix like systems.

A number of small changes designed to separate the actual socket handling from the rest of the logic in ThreadSocketHandler.

This is a simpler version of #14147
Move NotifyNumConnectionsChanged logic to private method. edb5350c32
Move DisconnectNodes logic to private method. 7479b63d91
Move InactivityCheck logic to private method. 2af9cff11a
Move SocketHandler logic to private method. 032488e6e7
DrahtBot commented at 10:27 PM on September 26, 2018: contributor
Reviewers, this pull request conflicts with the following ones:
- #14046 (net: Refactor message parsing (CNetMessage), adds flexibility by jonasschnelli)
- #14032 (Add p2p layer encryption with ECDH/ChaCha20Poly1305 by jonasschnelli)
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
pstratem cross-referenced this on Sep 26, 2018 from issue net: implement poll by pstratem
DrahtBot cross-referenced this on Sep 26, 2018 from issue wip: net: Implement poll by pstratem
DrahtBot cross-referenced this on Sep 26, 2018 from issue net: Refactor ThreadSocketHandler by pstratem
fanquake added the label P2P on Sep 26, 2018
MarcoFalke added the label Refactoring on Sep 26, 2018
DrahtBot cross-referenced this on Sep 26, 2018 from issue net: Refactor message parsing (CNetMessage), adds flexibility by jonasschnelli
DrahtBot cross-referenced this on Sep 26, 2018 from issue Add p2p layer encryption with ECDH/ChaCha20Poly1305 by jonasschnelli
ryanofsky commented at 5:26 PM on September 27, 2018: contributor

utACK 032488e6e7b4c2d37f3f434d575d25806ddcb0b0. PR description above should contain an actual description of the changes, though, and not just reference #14147. (This PR seems the same as #14147 except it replaces InactivityChecks with InactivityCheck and moves it below NotifyNumConnectionsChanged. I'm actually not sure why #14147 was closed and replaced instead of just updated.)
pstratem commented at 7:25 PM on September 27, 2018: contributor

@ryanofsky I wanted to simplify the changes to the point that they are as obviously correct as possible.

All of these changes are now just moving logic to functions.
DrahtBot commented at 2:20 AM on September 28, 2018: contributor



Coverage Change (pull 14335) Reference (master)

Lines +0.0012 % 87.0361 %

Functions +0.0252 % 84.1130 %

Branches -0.0026 % 51.5451 %
sipa commented at 7:57 PM on October 12, 2018: member

utACK 032488e6e7b4c2d37f3f434d575d25806ddcb0b0. Nice to see logic moved out of those giant blocks of code.
donaloconnor commented at 10:26 PM on October 12, 2018: contributor

utACK - looks much better 👍
meshcollider commented at 10:58 PM on October 12, 2018: contributor

utACK https://github.com/bitcoin/bitcoin/commit/032488e6e7b4c2d37f3f434d575d25806ddcb0b0, nice refactor
laanwj commented at 3:19 PM on October 16, 2018: member

utACK caa08261d1fa1e699b4e649b644dd4f3520de755
laanwj merged this on Oct 16, 2018
laanwj closed this on Oct 16, 2018
laanwj referenced this in commit 23419e4c49 on Oct 16, 2018
MarcoFalke commented at 10:05 PM on October 23, 2018: member

post-merge utACK 032488e6e7b4c2d37f3f434d575d25806ddcb0b0
codablock cross-referenced this on Apr 7, 2020 from issue Backport poll() code from Bitcoin by codablock
codablock referenced this in commit 3bcee4a480 on Apr 8, 2020
codablock referenced this in commit f9bf204829 on Apr 8, 2020
bitcoin locked this on Sep 8, 2021