Open GoogleCodeExporter opened 9 years ago
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
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
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
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
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:
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
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
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
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
Any progress on this? The submitted patch not OK for some reason?
Original comment by sberlin
on 5 Nov 2009 at 2:21
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
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
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
Original issue reported on code.google.com by
crazybob...@gmail.com
on 28 Feb 2009 at 6:33