Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

index: Create IndexRunner class for activing indexes. #14111

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
5 participants
@jimpo
Copy link
Contributor

commented Aug 30, 2018

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.

src/index/runner.h Outdated
class BaseIndex;

/**
* RAII interface for activing indexes to stay in sync with blockchain updates.

This comment has been minimized.

Copy link
@marcinja

marcinja Aug 30, 2018

Contributor

s/activing/activating

@jimpo jimpo force-pushed the jimpo:index-runner branch Sep 10, 2018

@jimpo jimpo force-pushed the jimpo:index-runner branch 2 times, most recently Nov 6, 2018

@DrahtBot DrahtBot removed the Needs rebase label Nov 6, 2018

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Nov 6, 2018

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

@jimpo jimpo force-pushed the jimpo:index-runner branch Nov 8, 2018

@jimpo

This comment has been minimized.

Copy link
Contributor Author

commented Nov 8, 2018

@MarcoFalke That was weird. Fixed it.

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Nov 9, 2018

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.

@bitcoin bitcoin deleted a comment from DrahtBot Nov 9, 2018

@bitcoin bitcoin deleted a comment from DrahtBot Nov 9, 2018

@bitcoin bitcoin deleted a comment from DrahtBot Nov 9, 2018

@bitcoin bitcoin deleted a comment from DrahtBot Nov 9, 2018

@laanwj laanwj added this to Blockers in High-priority for review Jan 3, 2019

@laanwj laanwj removed this from Blockers in High-priority for review Jan 3, 2019

@jimpo jimpo force-pushed the jimpo:index-runner branch 2 times, most recently to 0c67d14 Jan 9, 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.

@jimpo jimpo force-pushed the jimpo:index-runner branch from 0c67d14 to fa402af Feb 22, 2019

@DrahtBot DrahtBot removed the Needs rebase label Feb 22, 2019

@ryanofsky
Copy link
Contributor

left a comment

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

This comment has been minimized.

Copy link
Contributor Author

commented Mar 1, 2019

@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

This comment has been minimized.

Copy link
Contributor

commented Mar 2, 2019

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 2196c51 in #13743. Right now if I comment out txindex.Stop() in txindex_tests.cpp, tests still seem to pass.

@jimpo

This comment has been minimized.

Copy link
Contributor Author

commented Mar 2, 2019

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

@jimpo jimpo closed this Mar 2, 2019

@jimpo jimpo deleted the jimpo:index-runner branch Mar 2, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.