Disable Just-in-time bindings

[GoogleCodeExporter]

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 reported on code.google.com by crazybob...@gmail.com on 28 Feb 2009 at 6:33

[GoogleCodeExporter]

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() ?

Original comment by limpbizkit on 28 Feb 2009 at 9:30

• Changed title: Disable Just-in-time bindings
• Added labels: Priority-High

[GoogleCodeExporter]

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

Original comment by crazybob...@gmail.com on 28 Feb 2009 at 9:58

[GoogleCodeExporter]

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

Original comment by dolphin.wan@gmail.com on 3 Mar 2009 at 4:09

[GoogleCodeExporter]

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

Original comment by sberlin on 3 Mar 2009 at 4:11

[GoogleCodeExporter]

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.

Original comment by sberlin on 4 Oct 2009 at 8:29

Attachments:

• changes.txt

[GoogleCodeExporter]

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

Original comment by limpbizkit on 5 Oct 2009 at 3:16

[GoogleCodeExporter]

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.

Original comment by sberlin on 5 Oct 2009 at 2:17

[GoogleCodeExporter]

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

Original comment by crazybob...@gmail.com on 5 Oct 2009 at 3:02

[GoogleCodeExporter]

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

Original comment by sberlin on 5 Oct 2009 at 3:42

[GoogleCodeExporter]

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

Original comment by sberlin on 5 Nov 2009 at 2:21

[GoogleCodeExporter]

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

Original comment by Maaarti...@gmail.com on 8 Nov 2009 at 12:27

[GoogleCodeExporter]

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.

Original comment by sberlin on 17 Nov 2009 at 4:10

[GoogleCodeExporter]

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.

Original comment by sberlin on 11 Feb 2010 at 10:10

• Changed state: Fixed