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

config: v2 Address resolution using named resolver #1835

Merged
merged 16 commits into from
Oct 13, 2017
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Make RegisterFactory remove factory on delete
This will make it possible to use RegisterFactory in tests without
leaking state between tests.

Signed-off-by: Alex Konradi <[email protected]>
akonradi committed Oct 10, 2017
commit 3ed3f63347a15b4d544b3e9ec7c631655f92d73e
25 changes: 21 additions & 4 deletions include/envoy/registry/registry.h
Original file line number Diff line number Diff line change
@@ -46,6 +46,16 @@ template <class Base> class FactoryRegistry {
return it->second;
}

/**
* Remove a factory by name.
*/
static void unregisterFactory(Base& factory) {
auto result = factories().erase(factory.name());
if (result == 0) {
throw EnvoyException(fmt::format("No registration for name: '{}'", factory.name()));
}
}

private:
/**
* Gets the current map of factory implementations.
@@ -73,10 +83,17 @@ template <class T, class Base> class RegisterFactory {
/**
* Contructor that registers an instance of the factory with the FactoryRegistry.
*/
RegisterFactory() {
static T* instance = new T;
FactoryRegistry<Base>::registerFactory(*instance);
}
RegisterFactory() { FactoryRegistry<Base>::registerFactory(instance_); }

/**
* Destructor that removes an instance of the factory from the FactoryRegistry
*/
~RegisterFactory() { FactoryRegistry<Base>::unregisterFactory(instance_); }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This whole static registration facility is (already) violating the static initialization order fiasco rules in the style guide. We're adding another way that things can go wrong in terms of destructor ordering here, but since its only effect is to unregister I would argue it's safe.

Copy link
Member

@mrice32 mrice32 Oct 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This new setup creates a slightly bigger violation of the static ordering fiasco - we now have destructors being called in some unknown ordering (in addition to the constructors). @akonradi mentioned this was done so that tests don't leak state. Maybe it would be preferable to create an unregister method on the FactoryRegistry that manually removes the pointer (and allows the user to delete, maybe by returning as a std::unique_ptr) rather than doing it by default on destruction of the RegisterFactory class since we'd only need this in tests? @htuch thoughts?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@akonradi can you point at the specific test where the leak was an issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/akonradi/envoy/blob/915c7d3b432ae9a4abbc4f69da5bf19694feb59a/test/common/network/resolver_impl_test.cc#L145 is where there could have been one if the resolver from https://github.com/akonradi/envoy/blob/915c7d3b432ae9a4abbc4f69da5bf19694feb59a/test/common/network/resolver_impl_test.cc#L106 was allowed to leak. FWIW it seems like we've avoided this in the rest of the tests because we don't register test factories and stick to testing the included ones.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is where a singleton override for test would be useful. @alyssawilk any thoughts on how far along the work is to provide this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hasn't started (not assigned) but it looked like a pretty simple implementation. If it's as few lines of code as I think it could be folded in here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're talking about #1808? I'd rather make that a separate pull request and either wait to merge it once it lands in master or add a TODO here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, TODO sounds good to me, and update the issue to reference, thanks.


T& testGetFactory() { return instance_; }

private:
T instance_;
};

} // namespace Registry