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

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);

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

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.)

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

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!

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.

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.

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).

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

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

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....

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.

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