CConnman: move initialization to declaration

ajtowns commented at 12:59 pm on November 17, 2020: member

Ensure nMaxOutboundTotalBytesSentInCycle and nMaxOutboundCycleStartTime are initialized even if CConnman::Start() is not called. Prevents failures in test/fuzz/connman when run under valgrind.

ajtowns commented at 12:59 pm on November 17, 2020: member

cc @practicalswift @MarcoFalke ; related: #20188 (comment)

fanquake added the label P2P on Nov 17, 2020

in src/net.cpp:2450 in c401a1d83f outdated

2448@@ -2449,12 +2449,6 @@ bool CConnman::Start(CScheduler& scheduler, const Options& connOptions)
2449         LOCK(cs_totalBytesRecv);
2450         nTotalBytesRecv = 0;

MarcoFalke commented at 1:11 pm on November 17, 2020:

Any reason to not move this as well?

ajtowns commented at 1:16 pm on November 17, 2020:

No reason to move it: it’s already initialized to 0 at declaration, so it should already be safe; and moving it wouldn’t reduce the number of locks. No reason not to move it either though, I think.

ajtowns commented at 4:48 pm on November 17, 2020:

Moved others to initialize at declaration, and no longer reinitializing any of them in Start()

MarcoFalke approved

MarcoFalke commented at 1:12 pm on November 17, 2020: member

Seems fine. The alternative would be to call Start in the fuzz tests, but that spins up quite a few threads.

DrahtBot commented at 1:49 pm on November 17, 2020: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#20253 (net: use std::chrono throughout maxOutbound logic by fanquake)
#20234 (net: don’t extra bind for Tor if binds are restricted by vasild)

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.

MarcoFalke commented at 1:53 pm on November 17, 2020: member

review ACK c401a1d83f119f11fe0e446193458c5de224adbf

jnewbery commented at 2:48 pm on November 17, 2020: member

Is there any downside to just using default initialization for these data members?

0-    uint64_t nMaxOutboundTotalBytesSentInCycle GUARDED_BY(cs_totalBytesSent);
1+    uint64_t nMaxOutboundTotalBytesSentInCycle GUARDED_BY(cs_totalBytesSent) {0};
2-    uint64_t nMaxOutboundCycleStartTime GUARDED_BY(cs_totalBytesSent);
3+    uint64_t nMaxOutboundCycleStartTime GUARDED_BY(cs_totalBytesSent) {0};

It seems to me that Init() should only initialize members that require a data from the connOptions object.

practicalswift commented at 2:48 pm on November 17, 2020: contributor

Thanks for noticing and fixing this @ajtowns!

Any reason not to use in-class member initialisation?

Like this:

 0diff --git a/src/net.h b/src/net.h
 1index 77649247d..d8d7daf64 100644
 2--- a/src/net.h
 3+++ b/src/net.h
 4@@ -478,10 +478,10 @@ private:
 5     uint64_t nTotalBytesSent GUARDED_BY(cs_totalBytesSent) {0};
 6
 7     // outbound limit & stats
 8-    uint64_t nMaxOutboundTotalBytesSentInCycle GUARDED_BY(cs_totalBytesSent);
 9-    uint64_t nMaxOutboundCycleStartTime GUARDED_BY(cs_totalBytesSent);
10-    uint64_t nMaxOutboundLimit GUARDED_BY(cs_totalBytesSent);
11-    uint64_t nMaxOutboundTimeframe GUARDED_BY(cs_totalBytesSent);
12+    uint64_t nMaxOutboundTotalBytesSentInCycle GUARDED_BY(cs_totalBytesSent) {0};
13+    uint64_t nMaxOutboundCycleStartTime GUARDED_BY(cs_totalBytesSent) {0};
14+    uint64_t nMaxOutboundLimit GUARDED_BY(cs_totalBytesSent) {0};
15+    uint64_t nMaxOutboundTimeframe GUARDED_BY(cs_totalBytesSent) {0};
16
17     // P2P timeout in seconds
18     int64_t m_peer_connect_timeout;

Rationale provided in the C++ Core Guidelines: C.48: Prefer in-class initializers to member initializers in constructors for constant initializers :)

practicalswift commented at 2:50 pm on November 17, 2020: contributor

What @jnewbery said literally 4 seconds before me :)

MarcoFalke commented at 4:15 pm on November 17, 2020: member

I presumed that the Start method was provided so that connman could be stopped and started while core is running. Members would be re-initialized. Though, we don’t do that right now and I don’t think we will, so either approach is fine by me.

CConnman: initialise at declaration rather than in Start()

Ensure nMaxOutboundTotalBytesSentInCycle and nMaxOutboundCycleStartTime
are initialized even if CConnman::Start() is not called.

9d09132be4

ajtowns force-pushed on Nov 17, 2020

ajtowns renamed this:
~~CConnman: move initialization into Init()~~
CConnman: move initialization to declaration
on Nov 17, 2020

ajtowns commented at 4:49 pm on November 17, 2020: member

I think it would be a large change to make it reasonable to call Start() twice, so moved init to declaration, without resetting them in Start().

practicalswift commented at 8:42 pm on November 17, 2020: contributor

ACK 9d09132be4ff99f98ca905c342347d5f35f13350: patch looks correct!

MarcoFalke commented at 7:13 am on November 18, 2020: member

review ACK 9d09132be4ff99f98ca905c342347d5f35f13350 , checked that we call Start only once and in the same scope where connman is constructed (AppInitMain) 💸

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3review ACK 9d09132be4ff99f98ca905c342347d5f35f13350 , checked that we call Start only once and in the same scope where connman is constructed (AppInitMain) 💸
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUiqVQv/UC5Tg9YLXIPXSm3OwF820sVE+O+s6Ks4t71B/pzcwfUzq4llOxTx3TDW
 8nvpgLlCVeJkW79tUby/5YBVzrrgZ80KaGldO3H2eGvdi9ARSD3cX6OtokYNUHmty
 9ZJXijsEs6zlli3LK3wlBXZ30HNjkSzyLOK9CvnU9/k+MNwKvqSDYQRzHHMRungUU
10+fBuc2CVW16WVhgmXMWfQb7QWjMxuqe5JwD6PAuGPftgXYB48uHR8Uys36X1o6fn
11TQ1WqoZcDDULp4XIyRxqcC4FhTCBaacLE3ePZC/AzcP7Ga51brW9XuNZC8yFW4su
12omaUNR2Sw0XGTqtXD1BeIT00ZbkLyX7BtVi7VMgXVCIrjZBqdPXIK4PBqtydMk7g
13S5ocg3QVSLE8lZGPXg3va8u0cs7cu4ylxIyx1iIwDJmItFSv48EVGjk7CVdbWv+c
145zMC9m3XbyYQP8IP59Z2LVstQ8ga+dWCxECTTtT260gwl2CsSCFl59fL4jBWNHvu
15hUIVVCRj
16=EJrI
17-----END PGP SIGNATURE-----

python: error while loading shared libraries: libpython3.8.so.1.0: cannot open shared object file: No such file or directory

jnewbery commented at 10:58 am on November 18, 2020: member

Code review ACK 9d09132be4

fanquake merged this on Nov 18, 2020

fanquake closed this on Nov 18, 2020

sidhujag referenced this in commit 22154edcf5 on Nov 18, 2020

Fabcien referenced this in commit 8286ac28c0 on Jan 3, 2022

DrahtBot locked this on Feb 15, 2022

CConnman: move initialization to declaration #20408

Conflicts