index: Create IndexRunner class for activing indexes.

jimpo commented at 4:55 pm on August 30, 2018: contributor

As noted by @ryanofsky and mitigated in #13894 and #14071, the index destructor previously had a memory violation by accessing its own address in call to UnregisterValidationInterface. This is problematic even if Start is never called on the index. The new runner class is an RAII-style interface for sync thread management and validation interface registration.

I’m not sure if this is a good idea or an unnecessary abstraction since the init module still has responsibility for starting/stopping via global pointers. I do think it’s a better separation of concerns though. Looking for feedback before polishing up the PR.

in src/index/runner.h:15 in 4fb997095a outdated

10+#include <threadinterrupt.h>
11+
12+class BaseIndex;
13+
14+/**
15+ * RAII interface for activing indexes to stay in sync with blockchain updates.

marcinja commented at 7:04 pm on August 30, 2018:

s/activing/activating

jimpo force-pushed on Sep 10, 2018

DrahtBot added the label Needs rebase on Sep 12, 2018

jimpo force-pushed on Nov 6, 2018

DrahtBot removed the label Needs rebase on Nov 6, 2018

MarcoFalke commented at 8:20 pm on November 6, 2018: member

Doesn’t compile on xenial gcc and clang-3.2?

jimpo force-pushed on Nov 8, 2018

jimpo commented at 8:49 am on November 8, 2018: contributor

@MarcoFalke That was weird. Fixed it.

DrahtBot commented at 9:28 am on November 9, 2018: 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:

#10443 (Add fee_est tool for debugging fee estimation code by ryanofsky)

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.

fanquake deleted a comment on Nov 9, 2018

laanwj added this to the "Blockers" column in a project

laanwj removed this from the "Blockers" column in a project

jimpo force-pushed on Jan 9, 2019

jimpo force-pushed on Jan 10, 2019

DrahtBot added the label Needs rebase on Feb 16, 2019

index: Create IndexRunner class for activing indexes.

The index destructor previously had a memory violation by accessing
its own address in call to UnregisterValidationInterface. The new
runner class is an RAII-style interface for sync thread management
and validation interface registration.

fa402afc6d

jimpo force-pushed on Feb 22, 2019

DrahtBot removed the label Needs rebase on Feb 22, 2019

ryanofsky commented at 10:04 pm on March 1, 2019: member

Looking at this I’m struggling to see what problem it is supposed to solve. It seems to be pulling code out of the BaseIndex::Start and BaseIndex::Stop methods into a separate IndexRunner::IndexRunner class. But it isn’t clear what advantages this gives, since now for every BaseIndex instance that exists, there needs to be a parallel IndexRunner instance, and there needs to be a new global std::map, so you can find the IndexRunner pointer from the BaseIndex pointer.

This just seems more complicated than before, and of course requires more code. I see that this PR was created in response to a bug that either previously existed or previously could have existed. Is there still a bug that this can help prevent now? Or some other advantage this brings?

jimpo commented at 10:54 pm on March 1, 2019: contributor

@ryanofsky Thanks for taking a look.

The bug definitely still exists, so one way or another the call to Stop must be removed in the destructor of BaseIndex. This manifests for me in #14121.

The idea here is to have an RAII wrapper in the places where the control flow is not managed through globals. Concretely, the only place we see some benefit is in the unit tests, where the explicit Stop call can be removed. The global map is just there because in bitcoind, the index lifecycle is manages through global functions.

The much simpler alternative here is to just require that Stop be called before the index goes out of scope as part of its contract, which is pretty much as it is now (except that the Stop in the destructor can be removed). Conceptually, the RAII wrapper seems cleaner and makes it easier to handle exceptions in general, though it doesn’t seem to provide much value given how indexes are actually managed by bitcoind now.

ryanofsky commented at 0:10 am on March 2, 2019: member

Is this bug where UnregisterValidationInterface can’t be called from the destructor because it accesses the virtual function table, which is not valid during destruction? If so, I believe this should have been fixed by 2196c51821e340c9a9d2c76c30f9402370f84994 in #13743. Right now if I comment out txindex.Stop() in txindex_tests.cpp, tests still seem to pass.

jimpo commented at 9:53 pm on March 2, 2019: contributor

@ryanofsky Oh, amazing. While I still like this refactor aesthetically, it’s not worth it. Thanks!

jimpo closed this on Mar 2, 2019

jimpo deleted the branch on Mar 2, 2019

DrahtBot locked this on Feb 15, 2022

index: Create IndexRunner class for activing indexes. #14111

Conflicts