index: Create IndexRunner class for activing indexes. #14111
Conversation
src/index/runner.h
Outdated
jimpo
force-pushed
the
index-runner
branch
2 times, most recently
from
db6aac9 to
13e3561
Compare
|
Doesn't compile on xenial gcc and clang-3.2? |
|
@MarcoFalke That was weird. Fixed it. |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
jimpo
force-pushed
the
index-runner
branch
2 times, most recently
from
c7df45c to
0c67d14
Compare
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.
ryanofsky
left a comment
ryanofsky
left a comment
There was a problem hiding this 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?
|
@ryanofsky Thanks for taking a look. The bug definitely still exists, so one way or another the call to 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 The much simpler alternative here is to just require that |
|
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 |
|
@ryanofsky Oh, amazing. While I still like this refactor aesthetically, it's not worth it. Thanks! |
5 participants
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
Startis 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
initmodule 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.