CConnman: move initialization to declaration #20408

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?

-    uint64_t nMaxOutboundTotalBytesSentInCycle GUARDED_BY(cs_totalBytesSent);
+    uint64_t nMaxOutboundTotalBytesSentInCycle GUARDED_BY(cs_totalBytesSent) {0};
-    uint64_t nMaxOutboundCycleStartTime GUARDED_BY(cs_totalBytesSent);
+    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:

diff --git a/src/net.h b/src/net.h
index 77649247d..d8d7daf64 100644
--- a/src/net.h
+++ b/src/net.h
@@ -478,10 +478,10 @@ private:
     uint64_t nTotalBytesSent GUARDED_BY(cs_totalBytesSent) {0};

     // outbound limit & stats
-    uint64_t nMaxOutboundTotalBytesSentInCycle GUARDED_BY(cs_totalBytesSent);
-    uint64_t nMaxOutboundCycleStartTime GUARDED_BY(cs_totalBytesSent);
-    uint64_t nMaxOutboundLimit GUARDED_BY(cs_totalBytesSent);
-    uint64_t nMaxOutboundTimeframe GUARDED_BY(cs_totalBytesSent);
+    uint64_t nMaxOutboundTotalBytesSentInCycle GUARDED_BY(cs_totalBytesSent) {0};
+    uint64_t nMaxOutboundCycleStartTime GUARDED_BY(cs_totalBytesSent) {0};
+    uint64_t nMaxOutboundLimit GUARDED_BY(cs_totalBytesSent) {0};
+    uint64_t nMaxOutboundTimeframe GUARDED_BY(cs_totalBytesSent) {0};

     // P2P timeout in seconds
     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) 💸

<details><summary>Show signature and timestamp</summary>

Signature:

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

</details>

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