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

Option to discard bootstrap injector #102

Closed
wants to merge 5 commits into from
Closed

Option to discard bootstrap injector #102

wants to merge 5 commits into from

Conversation

elandau
Copy link
Contributor

@elandau elandau commented Mar 3, 2014

Using child injectors can be problematic in certain situations where
the injector must be injected (usually part of framework code with
dynamic class loading) resulting in the wrong injector being injected.
While there are some workarounds to this problem they are not obvious
and have issues of their own. This pull request make it possible to
have the LifecycleInjector discard the bootstrap injector thereby
making the injector returned by createInjector() a root injector.

Using child injectors can be problematic in certain situations where
the injector must be injected (usually part of framework code with
dynamic class loading) resulting in the wrong injector being injected.
While there are some workarounds to this problem they are not obvious
and have issues of their own.  This pull request make it possible to
have the LifecycleInjector discard the bootstrap injector thereby
making the injector returned by createInjector() a root injector.
@@ -30,7 +30,6 @@
private final List<Module> modules = Lists.newArrayList();

private static final String GUICE_PACKAGE_PREFX = "com.google.inject";
private static final String GOVERNATOR_PACKAGE_PREFIX = "com.netflix.governator.guice";

public InternalModuleDependencyModule() {
Copy link
Contributor

Choose a reason for hiding this comment

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

low-case file name sneaked in again?

@Randgalt
Copy link
Contributor

Randgalt commented Mar 6, 2014

Please detail the problematic situations and why this is needed.

…tor code into a new SimpleLifecycleInjector class
@elandau
Copy link
Contributor Author

elandau commented Mar 6, 2014

There are issues with JIT and the child injector in combination with Singletons. I'm specifically referring to things like https://groups.google.com/forum/#!topic/google-guice/2bznC6Al5DQ and https://groups.google.com/forum/#!topic/google-guice/Q592mWKTS1Q.

While using child injectors to strictly enforce creation order is nice I don't think forcing child injectors on users of Governator is absolutely necessary. The proposed solution simply provides an alternative way of doing things.

@Randgalt
Copy link
Contributor

Randgalt commented Mar 9, 2014

Is there a current problem/bug that you're trying to solve? The issues above have workarounds. This change significantly complicates Governator and imposes two implementations that have to be maintained. If there is a currently problem that needs to be solved let's solve that. If there isn't a current problem, I don't see the value of introducing a parallel implementation.

@elandau elandau closed this Mar 19, 2014
@elandau
Copy link
Contributor Author

elandau commented Mar 19, 2014

Fixed by #103

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants