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

Disable Just-in-time bindings #342

Closed
gissuebot opened this issue Jul 7, 2014 · 13 comments

Comments

@gissuebot
Copy link

@gissuebot gissuebot commented Jul 7, 2014

From crazyboblee on February 28, 2009 13:33:54

Automatically generating constructor bindings at run time oftentimes
results in subtle bugs. Many people prefer to explicitly bind everything in
their modules, and they want an error if they forget to bind something.

Original issue: http://code.google.com/p/google-guice/issues/detail?id=342

@gissuebot

This comment has been minimized.

Copy link
Author

@gissuebot gissuebot commented Jul 7, 2014

From limpbizkit on February 28, 2009 13:30:10

Most applications use a huge number of JIT bindings. Every time you do a linked binding:
  bind(Foo.class).to(RealFoo.class)
...usually there isn't a binding for RealFoo, and Guice creates a JIT binding for it.

Rather than throwing out JIT bindings (which are generally useful), I think we want to add a configuration to
requires the @Inject annotation. Perhaps binder.requireInjectAnnotation() ?

Summary: Disable Just-in-time bindings
Labels: Priority-High

@gissuebot

This comment has been minimized.

Copy link
Author

@gissuebot gissuebot commented Jul 7, 2014

From crazyboblee on February 28, 2009 13:58:06

I'm just talking about JIT bindings from the user's perspective, not how it's
implemented under the covers.

For bind(Foo).to(FooImpl), we'll materialize the JIT binding under the covers either way.

But, if FooImpl has a no-arg constructor and the user tries to inject FooImpl
somewhere w/o creating an explicit binding, they get an error. To get rid of the
error, they'd have to annotate FooImpl's constructor w/ @Inject or do this:

  bind(FooImpl.class);

@gissuebot

This comment has been minimized.

Copy link
Author

@gissuebot gissuebot commented Jul 7, 2014

From dolphin.wan on March 03, 2009 08:09:53

Looks like a user also wants to be explicit in specifying bindings: http://www.jroller.com/joshua_davis/entry/a_small_guice_gotcha

@gissuebot

This comment has been minimized.

Copy link
Author

@gissuebot gissuebot commented Jul 7, 2014

From sberlin on March 03, 2009 08:11:51

I'd like to disable bindings too, actually.  In my case, I'd like to disable anything
that doesn't have an explicit bind(...) call in the Module(s) the Injector is using.
 (This is different than anything that has an @Inject, because some code may have an
@Inject for use with other modules.)

@gissuebot

This comment has been minimized.

Copy link
Author

@gissuebot gissuebot commented Jul 7, 2014

From sberlin on October 04, 2009 13:29:39

Took a stab at this this weekend (because having it enables some nice optimizations
for child injectors not having to lookup bindings in parent injectors)..  Works
pretty much as expected.  Only downside is it adds a new ugly boolean parameter to a
bunch of methods in order for everything to work.  The basic idea is there's a new
Guice.createInjectorBuilder & Injector.createChildInjectorBuilder method that exposes
some methods including a new disableJit method.  That passes it off to the
InjectorShell.Builder which constructs the InjectorImpl with the jitDisabled
parameter.  There's a new JitBindingsTest to make sure things work.  Things that
changed to support jitDisabled:

 - InternalFactory.get has a new 'linked' method.  True if you're getting as a result
of a linked binding.  This is necessary so that implicit jit bindings still work
[bind(Interface.class).to(Impl.class) -- Impl is an implicit JIT binding]
FactoryProxy (linked bindings) & BoundProviderFactory (linked provider bindings) both
pass true.  Other callers pass false or pass on the parameter from their caller.

 - InjectorImpl.getJustInTimeBinding & getBindingOrThrow has a new 'allowJit'
parameter which will let it fail if JIT bindings aren't allowed from the caller and
JIT is disabled.  InjectorImpl.createUnitializedBinding has a new 'jitBinding'
parameter, so that a ConstructorBindingImpl knows if it's created by a JIT binding,
to let its InternalFactory fail if it was constructed through a JIT binding & JIT was
disabled.

 - ConstructorBindingImpl.create & Factory take a new 'failIfNotLinked' parameter
(the Factory takes a few more, to create a good looking error message) so that an
implicit JIT binding will refuse to create itself if it wasn't retrieved through a
linked binding.

To apply the patch, first copy InjectorBuilder to InjectorBuilderImpl, then apply the
patch.  (The patch will delete internal.InjectorBuilder, add an interface in the
parent package, and tweak some parts of InjectorBuilderImpl.  I couldn't figure out
how to tell svn diff to include renaming... am more of a CVS user.)

I tested the patch out on LimeWire's codebase & we had ~100 JIT bindings that failed,
as expected.  Would be great to fix this, turn JIT off, and let child injectors be a
little better about not creating JIT bindings in the parent.

Attachment: gist
   changes.txt

@gissuebot

This comment has been minimized.

Copy link
Author

@gissuebot gissuebot commented Jul 7, 2014

From limpbizkit on October 04, 2009 20:16:56

Sam, this is awesome. I'm going to catch up on Guice stuff on Monday; this will certainly
be the highlight!

@gissuebot

This comment has been minimized.

Copy link
Author

@gissuebot gissuebot commented Jul 7, 2014

From sberlin on October 05, 2009 07:17:24

Re: Bob's comment, "Could we just add a disableJitBindings() method to Binder?" --
IMO, this is a kind of configuration thing more akin to setting the Stage.  If it
were possible to enable/disable JIT on a per-module basis, then it'd go well into a
Module (but JIT fundamentally is outside the scope of a single Module is applies to
the whole Injector, post creation).  It'd be strange to install a separate module
that has a side effect of disabling JIT, or to add disableJIT into a
Modules.overridden module.  There's a separate issue ( issue 395 ) for integrating an
InjectorBuilder, so this solves that one nicely too.

Re: some other things..  I realized last night that there's a small oversight in the
current patch.  Injector.getBinding calls getBindingOrThrow with 'allowJit' true
because many extensions & utility code use it to introspect bindings (including the
bindings that LinkedBindings link to, which are usually all JIT bindings).  In the
current patch, allowJit lets you create JIT bindings as well as retrieve existing
ones.  The allowJit parameter either should become an enum { NO_JIT, EXISTING_JIT,
NEW_OR_EXISTING_JIT } or another parameter should be added (to accomplish the same
purpose).  getBinding should use EXISTING_JIT, whereas BoundProviderFactory &
FactoryProxy should use NEW_OR_EXISTING_JIT.  getBindingOrThrow would pass that off
to getJustInTimeBinding, and it'd fail on top of the enum was NO_JIT and a new check
after the synchronized block if it wasn't NEW_OR_EXISTING_JIT.

Without the change, getBinding can be used to bypass disabling of JIT bindings.

@gissuebot

This comment has been minimized.

Copy link
Author

@gissuebot gissuebot commented Jul 7, 2014

From crazyboblee on October 05, 2009 08:02:55

The Stage param predates Binder. I actually do wish it was set on the Binder, too. The
Binder API scales better than method overloading.

@gissuebot

This comment has been minimized.

Copy link
Author

@gissuebot gissuebot commented Jul 7, 2014

From sberlin on October 05, 2009 08:42:19

I suppose it could find a home next to Scopes & the like.  Still think it'd be a
little weird to have a module disable JIT bindings & another enable, and another
disable, etc..  And then there's a question of how the disabling propagates down to
child injectors (IMO it a child injector should by default use its parent's property,
but give the option to change it, but that may cause weird behavior).

@gissuebot

This comment has been minimized.

Copy link
Author

@gissuebot gissuebot commented Jul 7, 2014

From sberlin on November 05, 2009 06:21:00

Any progress on this?  The submitted patch not OK for some reason?

@gissuebot

This comment has been minimized.

Copy link
Author

@gissuebot gissuebot commented Jul 7, 2014

From Maaartinus on November 07, 2009 16:27:57

I've got a probably naive idea: What about binding a JitBindingListener? It could do
nothing (thus preserving the current behaviour), record all JIT Bindings (so users
could find out whats going on), or throw an Exception (thus disabling JIT Bindings).
The behaviour in case of overriding modules could be just as expected overriden....

@gissuebot

This comment has been minimized.

Copy link
Author

@gissuebot gissuebot commented Jul 7, 2014

From sberlin on November 17, 2009 08:10:25

All -- are there any plans to incorporate this patch (or something similar to it)?
We discovered a situation last night where we're accidentally creating JIT bindings
when we didn't want to.  (We have two injectors.. a core & a ui injector, and thought
we had replicated all necessary bindings to the ui injector, but it turns out we
didn't, and ended up recreating some singletons.)  Disabling JIT binding would have
prevented any problems (and warned us earlier that something was wrong).  If there's
no plans, I'll probably create a local fork of Guice again with the patch... but if
there are plans, I'll wait until it's merged.

Re: binding listeners, take a look at issue 387 .  The patch doesn't distinguish
between jit & non-jit, but it'd be easy to add.

@gissuebot

This comment has been minimized.

Copy link
Author

@gissuebot gissuebot commented Jul 7, 2014

From sberlin on February 11, 2010 14:10:50

fixed in r1141 .  right now an InjectorBuilder is used to expose the ability.  that
can change if the API doesn't work out, but the internals are all good to go.

Status: Fixed
Owner: sberlin

@gissuebot gissuebot closed this Jul 7, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
1 participant
You can’t perform that action at this time.