Stop using boost::signals2 internally in validationinterface. Replace with std::list and Add/Remove/Clear/Iterate helper functions.
Motivation for change is to reduce dependencies and avoid issues happening with boost versions before 1.59: #18517, https://github.com/bitcoin/bitcoin/pull/18471
Stop using boost::signals2 internally in validationinterface. Replace with
std::list and Add/Remove/Clear/Iterate helper functions.
Motivation for change is to reduce dependencies and avoid issues happening with
boost versions before 1.59:
https://github.com/bitcoin/bitcoin/issues/18517
https://github.com/bitcoin/bitcoin/pull/18471
pr/nosig.1 -> pr/nosig.2, compare) avoiding unneeded shared_ptr copies and cleaning up typedefs
Updated ad067a98ea1ca383898bf26d4abd00981246471f -> 01639a21d12df54895d0214542b84335d7f58a94 (pr/nosig.2 -> pr/nosig.3, compare) removing last typedef
52 explicit MainSignalsInstance(CScheduler *pscheduler) : m_schedulerClient(pscheduler) {}
53+
54+ void Add(std::shared_ptr<CValidationInterface> callbacks)
55+ {
56+ LOCK(m_mutex);
57+ auto inserted = m_map.emplace(callbacks.get(), m_list.end());
re: #18524 (review)
So we add the same twice?
There’s two emplaces because of the map and list (if that’s the question)
assert(inserted.second)?
re: #18524 (review)
I mean this is allowing adding the same callbacks. Why not
assert(inserted.second)?
Unit tests fail if the unregister function is not idempotent, so it seems good to me that the register function is idempotent as well. I could imagine an idempotent API here making calling code simpler, even though I could also imagine an assert catching potential bugs. I think asserts tend to work a better at short range within a module instead of being used to make an API rigid in an attempt to catch external bugs. But I don’t think there is a right answer here and would be ok with a PR that took a different approach.
std::fstream::open is not idempotent, fails if file is already opened. I’d rather fix unit tests. Just an opinion and should not prevent this change going forward.
re: #18524 (review)
Personally I think these shouldn’t be idempotent because makes sense to call just once. For instance
std::fstream::openis not idempotent, fails if file is already opened. I’d rather fix unit tests. Just an opinion and should not prevent this change going forward.
Feel free to open a followup. If I wanted to change calling code not to use the same pointer more than once, I would change these to return bool and have calling code assert that they return true, rather having these crash on cases they can reasonably handle and making assumptions about calling code. I do find API’s like std::map’s insert/erase that are idempotent convenient for avoiding boilerplate code, so that’s another reason I like the keeping the boost behavior beyond not wanting to increase scope of this PR and not liking asserts across an api boundary
81+ m_map.clear();
82+ }
83+
84+ template<typename F> void Iterate(F&& f)
85+ {
86+ WAIT_LOCK(m_mutex, lock);
re: #18524 (review)
How about local copy and then iterate it lock free?
I’m not sure what approach could be entirely lock free since the list is global and can be modified from any thread.
I just implemented something simple that can be changed and optimized in the future. The lock is not held when calling CValidation interface methods. It avoids copying so allocations aren’t needed just to iterate the list. Assumption is that the list is iterated frequently and modified infrequently
Oops, I wanted to delete my own comment, but it seems I misclicked and deleted @promag’s. Restoring from mail:
Assumption is that the list is iterated frequently and modified infrequently
Right. What I had in mind is 2 lists and an atomic book list_changed. On iterating it would then sync the list with the mutex locked.
:) I was wondering what happened. So draft:
0if (list_changed) {
1 lock;
2 iterate_list = list;
3 list_changed = false
4}
5for (f : iterate_list) f()
re: #18524 (review)
Right. What I had in mind is 2 lists and an atomic book list_changed. On iterating it would then sync the list with the mutex locked.
I don’t understand the suggestion from the pseudocode. I don’t love the idea of having multiple lists, but even if you have them, I don’t see how you avoid locks copying the list if you do create copies, or avoid needing a condition variable to delay modifying the list if you don’t create copies. I’m probably just making an incorrect assumption about what you want to accomplish here. Feel free to post sample code, or just change the implementation in an alternate PR or followup PR. I’d be especially interested if it’s a simplification and not just an optimization.
re: #18524 (review)
Is it not possible that there are two invocations of Iterate simultaneously?
It shouldn’t be the most common thing but should be possible because notifications like CMainSignals::BlockChecked and CMainSignals::NewPoWValidBlock run on calling thread. Other notifications are sent from the scheduler so shouldn’t happen simultaneously.
Is it not possible that there are two invocations of Iterate simultaneously?
My previous suggestion isn’t possible in this case.
Tested that this fixes the issue. I also prefer dropping boost signals2.
I was going a different way. From https://stackoverflow.com/a/2265979
I suggest you go the other way around and connect a dummy slot, then disconnect it when your “real” slot is invoked. Connecting another slot will clean up stale connections, so your slot should be released.
Tested 01639a21d12df54895d0214542b84335d7f58a94 on Ubuntu 16.04.6 LTS, it works as expected.
The end of output:
0...
12020-04-04T20:13:51Z FlushStateToDisk: write coins cache to disk (0 coins, 0kB) started
22020-04-04T20:13:51Z FlushStateToDisk: write coins cache to disk (0 coins, 0kB) completed (0.00s)
32020-04-04T20:13:51Z [default wallet] Releasing wallet
42020-04-04T20:13:51Z Shutdown: done
The only concern is about failed CentOS 7 build on Travis.
82+ }
83+
84+ template<typename F> void Iterate(F&& f)
85+ {
86+ WAIT_LOCK(m_mutex, lock);
87+ for (auto it{m_list.begin()}, prev{m_list.end()};; prev = it++) {
I think some C++ compilers don’t allow to use auto in combination with C++11 list initialization for non-primitive types. You might have to write
0 for (auto it = m_list.begin(), prev{m_list.end()};; prev = it++) {
I think some C++ compilers don’t allow to use
autoin combination with C++11 list initialization for non-primitive types.
I think they allow to do it, but the deducted type is std::initializer_list in this case, which is not what we want here.
re: #18524 (review)
I think they allow to do it, but the deducted type is
std::initializer_listin this case, which is not what we want here.
This seems to be causing compile errors on centos 7:
https://travis-ci.org/github/bitcoin/bitcoin/jobs/671000374#L2207
Changed {} to () to see if it helps with that compiler
pr/nosig.3 -> pr/nosig.4, compare) to fix centos 7 compiler error
77+ LOCK(m_mutex);
78+ for (auto it = m_list.begin(); it != m_list.end();) {
79+ if (--it->count == 0) it = m_list.erase(it); else ++it;
80+ }
81+ m_map.clear();
82+ }
It follows from the code of the Clear() function that m_list could be non-empty when the function returns.
Is it consistent state if m_list.empty() == false and m_map.empty() == true ?
re: #18524 (review)
It follows from the code of the
Clear()function thatm_listcould be non-empty when the function returns.Is it consistent state if
m_list.empty() == falseandm_map.empty() == true?
Yes, this happens when the reference count of one or more callbacks in the list is nonzero, which means they are currently being called, and will be removed by the iterate function when they are done being called, clearing the list.
Updated 3d463addfe2859bc55916f61086aaab624132411 -> b5fea244e5ac38758d295203209b66bd5905d714 (pr/nosig.4 -> pr/nosig.5, compare) to destroy scheduler before callback list (no real change but makes more sense logically)
Also I’m noticing random appveyor test failures with OSError: [WinError 10048] Only one usage of each socket address (protocol/network address/port) is normally permitted https://ci.appveyor.com/project/DrahtBot/bitcoin/builds/31959888#L3892, I’m assuming not related to the PR
57+ auto inserted = m_map.emplace(callbacks.get(), m_list.end());
58+ if (inserted.second) {
59+ m_list.emplace_back();
60+ inserted.first->second = std::prev(m_list.end());
61+ }
62+ inserted.first->second->callbacks = std::move(callbacks);
callbacks are moved into the m_list always, but not only when an insertion into m_map occurs?
re: #18524 (review)
Why
callbacksare moved into them_listalways, but not only when an insertion intom_mapoccurs?
Shouldn’t make a difference in practice, but it seemed better to me to call operator=(&&) and do empty destroy on the RHS object consistently instead of having different call sequences depending on a basically unrelated condition. Also, this will probably never matter but maybe always updating could be useful and worth guaranteeing in the future if there’s internal property of the pointer worth updating like a new custom deleter.
38- boost::signals2::signal<void (const CBlockLocator &)> ChainStateFlushed;
39- boost::signals2::signal<void (const CBlock&, const BlockValidationState&)> BlockChecked;
40- boost::signals2::signal<void (const CBlockIndex *, const std::shared_ptr<const CBlock>&)> NewPoWValidBlock;
41-
42+ Mutex m_mutex;
43+ struct Entry { std::shared_ptr<CValidationInterface> callbacks; int count = 1; };
Am I correct in stating the invariants here?
Perhaps it’s worth spelling these out.
Am I correct in stating the invariants here?
Yes, that’s all correct and well stated. I’ll add this as a comment
One way to test this change is to load and unload wallets repeatedly like in #18362. Each time a wallet is loaded and unloaded, a CValidationInterface instance gets registered and unregistered. If this is done during a sync when there are lots of UpdateTip and BlockConnected callbacks, it can be a good way to mix registrations with notifications and try to trigger deadlocks & segfaults we’ve previously seen here. Some command lines I used for testing this:
0src/bitcoind -testnet -debug=1 -debugexclude=libevent -server=1 -printtoconsole
1
2src/bitcoin-cli -testnet createwallet w1
3src/bitcoin-cli -testnet unloadwallet w1
4src/bitcoin-cli -testnet createwallet w2
5src/bitcoin-cli -testnet unloadwallet w2
6
7while src/bitcoin-cli -testnet loadwallet w1 && src/bitcoin-cli -testnet unloadwallet w1; do true; done
8
9while src/bitcoin-cli -testnet loadwallet w2 && src/bitcoin-cli -testnet unloadwallet w2; do true; done
74+
75+ void Clear()
76+ {
77+ LOCK(m_mutex);
78+ for (auto it = m_list.begin(); it != m_list.end();) {
79+ if (--it->count == 0) it = m_list.erase(it); else ++it;
else on the same line like this without braces is hard to read (and easily changed into something with different semantics).
Nit: I think
elseon the same line like this without braces is hard to read (and easily changed into something with different semantics).
Changed to assign statement here. You’re right the else is easy to miss
97+ }
98+
99+ template<typename F> void Iterate(F&& f)
100+ {
101+ WAIT_LOCK(m_mutex, lock);
102+ for (auto it(m_list.begin()), prev(m_list.end());; prev = it++) {
Is this loop equivalent to the following?
0WAIT_LOCK(m_mutex, lock);
1for (auto it = m_list.begin(); it != m_list.end();) {
2 ++it->count;
3 {
4 REVERSE_LOCK(lock);
5 f(*it->callbacks);
6 }
7 it = (--it->count) ? std::next(it) : m_list.erase(it);
8}
Is this loop equivalent to the following?
Yes and that’s more straightforward. Updated
Updated b5fea244e5ac38758d295203209b66bd5905d714 -> c3a471604b1ef263d32c7d90db27b2fcfadc7166 (pr/nosig.5 -> pr/nosig.6, compare) adding suggested comments and renaming a few things to match comments.
Out of curiosity, have you considered a read write lock?
I was thinking about one in the context of the solution you were proposing. If we just used a read-write lock in the most straightforward way here without reference counts registering and unregistering would block while callbacks were executing which would be changing behavior from boost signals and might lead to external deadlocks
pr/nosig.6 -> pr/nosig.7, compare) simplifying iterate loop with sipa’s suggestion
66+ {
67+ LOCK(m_mutex);
68+ auto inserted = m_map.emplace(callbacks.get(), m_list.end());
69+ if (inserted.second) {
70+ m_list.emplace_back();
71+ inserted.first->second = std::prev(m_list.end());
inserted.first->second = m_list.emplace(m_list.end());.
re: #18524 (review)
I think these lines can be combined into
inserted.first->second = m_list.emplace(m_list.end());.
Thanks, switched to this
44- boost::signals2::signal<void (const CTransactionRef &)> TransactionRemovedFromMempool;
45- boost::signals2::signal<void (const CBlockLocator &)> ChainStateFlushed;
46- boost::signals2::signal<void (const CBlock&, const BlockValidationState&)> BlockChecked;
47- boost::signals2::signal<void (const CBlockIndex *, const std::shared_ptr<const CBlock>&)> NewPoWValidBlock;
48-
49+ Mutex m_mutex;
re: #18524 (review)
Do all of these member variables/types need to be public?
No none do, added private/public sections
Iterate which results in a different order of callback execution. However I don’t see how this could be a problem.
Updated 96176004a39c63fdd4ada1f07e55bc62ca0c447b -> d6815a2313158862d448733954a73520f223deb6 (pr/nosig.7 -> pr/nosig.8, compare) just adding public/private, simplifying emplace, and making erase calls more consistent
re: #18524#pullrequestreview-388062749
There is one little difference from boost signals2 when some validation interface is registered in the middle of some
Iteratewhich results in a different order of callback execution. However I don’t see how this could be a problem.
Curious what the boost behavior is when registrations change during a call. Seems like it would have to make a snapshot copy or use a kind of versioning to be able to ignore additions / removals
Curious what the boost behavior is when registrations change during a call. Seems like it would have to make a snapshot copy or use a kind of versioning to be able to ignore additions / removals
Yes, that’s the case, slots are called without any lock using a local list.
63 SingleThreadedSchedulerClient m_schedulerClient;
64- std::unordered_map<CValidationInterface*, ValidationInterfaceConnections> m_connMainSignals;
65
66 explicit MainSignalsInstance(CScheduler *pscheduler) : m_schedulerClient(pscheduler) {}
67+
68+ void Register(std::shared_ptr<CValidationInterface> callbacks)
50+ Mutex m_mutex;
51+ //! List entries consist of a callback pointer and reference count. The
52+ //! count is equal to the number of current executions of that entry, plus 1
53+ //! if it's registered. It cannot be 0 because that would imply it is
54+ //! unregistered and also not being executed (so shouldn't exist).
55+ struct ListEntry { std::shared_ptr<CValidationInterface> callbacks; int count = 1; };
int count{1};
76+ void Unregister(CValidationInterface* callbacks)
77+ {
78+ LOCK(m_mutex);
79+ auto it = m_map.find(callbacks);
80+ if (it != m_map.end()) {
81+ if (!--it->second->count) m_list.erase(it->second);
== 0.
ACK d6815a2313158862d448733954a73520f223deb6.
Tested it fixes #18517. Some nits for your consideration if you happen to push again.
While reviewing I thought it could make sense to define MainSignalsInstance destructor that would assert nothing is registered.
ACK d6815a2313158862d448733954a73520f223deb6
I like the general direction of this (to move away from boost::signals2), and as it works around a problem with boost I think this is the preferable way to fix this (both in 0.20 and master).
While reviewing I thought it could make sense to define MainSignalsInstance destructor that would assert nothing is registered.
FWIW asserts in destructors have turned out to be pretty terrible in generating shutdown crashes in unexpected conditions such as errors. So I’m not sure I like this.
FWIW asserts in destructors have turned out to be pretty terrible in generating shutdown crashes in unexpected conditions such as errors. So I’m not sure I like this.
It could be something that is only enabled on --enable-debug for tests, something like #16136
88+ //! are currently executing, but it will be cleared when they are done
89+ //! executing.
90+ void Clear()
91+ {
92+ LOCK(m_mutex);
93+ for (auto it = m_list.begin(); it != m_list.end();) {
Labels
Bug
Refactoring
Validation
Milestone
0.20.0