search

google-code-export / google-guice

Automatically exported from code.google.com/p/google-guice
Apache License 2.0
2 stars 1 forks source link

BytecodeGen uses system classloader when applying AOP to system types #343

Open GoogleCodeExporter opened 9 years ago

GoogleCodeExporter commented 9 years ago 
We currently assume that classloader bridging is not required for types
from the system classpath. For such cases the BytecodeGen.getClassLoader()
method just returns the type's classloader, which will be the system
classloader.

While this assumption is correct most of the time, it won't work when an
application loads Guice in a non-system classloader and then proceeds to
apply AOP to a system type. In this scenario the system classloader won't
have access to the internal AOP types, leading to a runtime exception.

This situation appears to be uncommon, because I've only just come across
one example of this very recently. Because fixing this requires changing
the algorithm in BytecodeGen (which would then entail re-testing) I would
be OK with this being postponed until 2.1 to avoid further slippage of Guice 2.

Original issue reported on code.google.com by mccu...@gmail.com on 3 Mar 2009 at 1:06

GoogleCodeExporter commented 9 years ago 
My proposed fix is as follows:

 * if the type has the same classloader as the BytecodeGen class we use that

 * if the type classloader is already a bridging classsloader we use that

 * if enabled and it's not a package-private type we use a bridge classloader

 * otherwise we use the type classloader

I'll attach a patch shortly along with a new test for this situation.

Original comment by mccu...@gmail.com on 3 Mar 2009 at 1:15

GoogleCodeExporter commented 9 years ago 
Suggested patch along with an update to the BytecodeGen testcases.

Because we now use a bridging classloader when the type classloader is 
different to
the Guice classloader I had to change 
testSystemClassLoaderIsUsedIfProxiedClassUsesIt
to be testGuiceClassLoaderIsUsedIfProxiedClassUsesIt. This is because in the 
testcase
the Guice classloader is different to the system classloader.

Original comment by mccu...@gmail.com on 3 Mar 2009 at 2:57

Attachments:

GoogleCodeExporter commented 9 years ago 
This is an enhanced patch that adds the ability to turn on bridge classloading 
even
when the type classloader is the same as the Guice classloader. This might be 
useful
when you want proxy classes unloaded as eagerly as possible (note because Guice 
now
shares proxy classes this is less of an issue than before).

To turn on eager bridge classloading use:  -Dguice.custom.loader=eager

Original comment by mccu...@gmail.com on 3 Mar 2009 at 3:35

Attachments:

GoogleCodeExporter commented 9 years ago 
FYI, this is the original thread discussing the problem:

  http://groups.google.com/group/guice-osgi/browse_thread/thread/a1030d7500a29dec

Original comment by mccu...@gmail.com on 3 Mar 2009 at 4:38

GoogleCodeExporter commented 9 years ago 
Latest patch that also wraps the call to getSystemClassLoader in a doPrivileged 
block.

Original comment by mccu...@gmail.com on 3 Mar 2009 at 5:40

Attachments:

GoogleCodeExporter commented 9 years ago 
I'd like to get this patch into Guice 2.1 if possible.

Original comment by mccu...@gmail.com on 30 Mar 2009 at 10:57

GoogleCodeExporter commented 9 years ago 
I'll patch this in.

Original comment by limpbizkit on 2 Apr 2009 at 6:25

GoogleCodeExporter commented 9 years ago

Original comment by limpbizkit on 26 Apr 2009 at 9:36

GoogleCodeExporter commented 9 years ago 
Stuart I'm looking at this CL. Wow! I'm still lost on a few things....

Why do we need THREE modes for the guice.custom.loader property? Either you're 
in OSGi/J2EE and you want 
custom loading, or you're not. When wouldn't these two be sufficient?

Could you write a test? I'm tempted to tidy up the code, but I don't want to 
break anything!

Original comment by limpbizkit on 9 May 2009 at 4:34

GoogleCodeExporter commented 9 years ago 
Originally we supported just two modes:

  <unset>/"true" - only use bridge classloaders where needed (and possible)
  "false" - never use bridge classloaders (pre-OSGi behaviour)

but I think it would be useful to support:

  "eager" - always use bridge classloaders wherever possible

This is different from the <unset> case because it will still try and use a 
bridge
classloader even if Guice and the class being proxied are in the same 
classloader.
This means that the proxy class can be eagerly unloaded even while the 
application
classloader is still active - without this mode you wouldn't be able to unload 
proxy
classes when proxying system types.

Now this is really a "nice-to-have", so if you think it will confuse people we 
can
simply drop it :) or we can change it to "false", "lazy", and "true" - with 
"lazy"
being the current default.

The other change is to support situations where we can't get the system 
classloader
(ie. the classloader equivalent to "null") like on the AppEngine, but we still 
want
to represent it as a non-null key in the map so we map it to a placeholder 
object.

Feel free to tidy up the patch while I write up a test for this change - you can
always attach the new patch to this issue in case you're finished before I am ;)

Original comment by mccu...@gmail.com on 10 May 2009 at 12:17

GoogleCodeExporter commented 9 years ago 
I'd like to make things work with only the two modes. I'll try to tweak your 
patch to handle this.

Original comment by limpbizkit on 13 May 2009 at 7:38

GoogleCodeExporter commented 9 years ago 
This patch didn't make it into 2.0, so rescheduling it for 2.1

Original comment by mccu...@gmail.com on 21 May 2009 at 9:02

GoogleCodeExporter commented 9 years ago 
I'd like to know whether the following is fixed by this patch:

I wrote a custom classloader to speed up the cold startup of LimeWire which is 
mostly
bound by file io caused by classloading.

I'm running into the following exception:

SEVERE: Application class org.limewire.ui.swing.mainframe.AppFrame failed to 
launch
com.google.inject.CreationException: Guice creation errors:

1) Error injecting method, com.google.inject.internal.ComputationException:
com.google.inject.internal.ComputationException:
com.google.inject.internal.cglib.core.CodeGenerationException:
java.lang.reflect.InvocationTargetException-->null
  at
com.google.inject.assistedinject.FactoryProvider2.initialize(FactoryProvider2.ja
va:161)
  at
org.limewire.ui.swing.search.LimeWireUiSearchModule.configure(LimeWireUiSearchMo
dule.java:56)

1 error
    at
com.google.inject.internal.Errors.throwCreationExceptionIfErrorsExist(Errors.jav
a:358)
    at
com.google.inject.internal.InjectorBuilder.injectDynamically(InjectorBuilder.jav
a:174)
    at com.google.inject.internal.InjectorBuilder.build(InjectorBuilder.java:114)
    at com.google.inject.Guice.createInjector(Guice.java:93)
    at com.google.inject.Guice.createInjector(Guice.java:81)
    at org.limewire.ui.swing.mainframe.AppFrame.createUiInjector(AppFrame.java:309)
    at org.limewire.ui.swing.mainframe.AppFrame.startup(AppFrame.java:153)
    at org.jdesktop.application.Application$1.run(Application.java:171)
    at java.awt.event.InvocationEvent.dispatch(InvocationEvent.java:209)
    at java.awt.EventQueue.dispatchEvent(EventQueue.java:597)
    at java.awt.EventDispatchThread.pumpOneEventForFilters(EventDispatchThread.java:273)
    at java.awt.EventDispatchThread.pumpEventsForFilter(EventDispatchThread.java:183)
    at java.awt.EventDispatchThread.pumpEventsForHierarchy(EventDispatchThread.java:173)
    at java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:168)
    at java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:160)
    at java.awt.EventDispatchThread.run(EventDispatchThread.java:121)
Caused by: com.google.inject.internal.ComputationException:
com.google.inject.internal.ComputationException:
com.google.inject.internal.cglib.core.CodeGenerationException:
java.lang.reflect.InvocationTargetException-->null
    at com.google.inject.internal.MapMaker$StrategyImpl.compute(MapMaker.java:553)
    at com.google.inject.internal.MapMaker$StrategyImpl.compute(MapMaker.java:419)
    at
com.google.inject.internal.CustomConcurrentHashMap$ComputingImpl.get(CustomConcu
rrentHashMap.java:2041)
    at com.google.inject.internal.FailableCache.get(FailableCache.java:46)
    at
com.google.inject.internal.ConstructorInjectorStore.get(ConstructorInjectorStore
.java:50)
    at
com.google.inject.internal.ConstructorBindingImpl.initialize(ConstructorBindingI
mpl.java:62)
    at com.google.inject.internal.InjectorImpl.initializeBinding(InjectorImpl.java:370)
    at com.google.inject.internal.BindingProcessor$1$1.run(BindingProcessor.java:164)
    at
com.google.inject.internal.BindingProcessor.initializeBindings(BindingProcessor.
java:219)
    at
com.google.inject.internal.InjectorBuilder.initializeStatically(InjectorBuilder.
java:121)
    at com.google.inject.internal.InjectorBuilder.build(InjectorBuilder.java:106)
    at com.google.inject.internal.InjectorImpl.createChildInjector(InjectorImpl.java:137)
    at com.google.inject.internal.InjectorImpl.createChildInjector(InjectorImpl.java:144)
    at
com.google.inject.assistedinject.FactoryProvider2.getBindingFromNewInjector(Fact
oryProvider2.java:203)
    at
com.google.inject.assistedinject.FactoryProvider2.initialize(FactoryProvider2.ja
va:171)
    at
com.google.inject.assistedinject.FactoryProvider2$$FastClassByGuice$$9dcdf6d7.in
voke(<generated>)
    at com.google.inject.internal.cglib.reflect.FastMethod.invoke(FastMethod.java:53)
    at
com.google.inject.internal.SingleMethodInjector$1.invoke(SingleMethodInjector.ja
va:55)
    at com.google.inject.internal.SingleMethodInjector.inject(SingleMethodInjector.java:87)
    at
com.google.inject.internal.MembersInjectorImpl.injectMembers(MembersInjectorImpl
.java:96)
    at com.google.inject.internal.MembersInjectorImpl$1.call(MembersInjectorImpl.java:73)
    at com.google.inject.internal.MembersInjectorImpl$1.call(MembersInjectorImpl.java:72)
    at com.google.inject.internal.InjectorImpl.callInContext(InjectorImpl.java:801)
    at
com.google.inject.internal.MembersInjectorImpl.injectAndNotify(MembersInjectorIm
pl.java:71)
    at com.google.inject.internal.Initializer$InjectableReference.get(Initializer.java:142)
    at com.google.inject.internal.Initializer.injectAll(Initializer.java:89)
    at
com.google.inject.internal.InjectorBuilder.injectDynamically(InjectorBuilder.jav
a:172)
    ... 14 more
Caused by: com.google.inject.internal.ComputationException:
com.google.inject.internal.cglib.core.CodeGenerationException:
java.lang.reflect.InvocationTargetException-->null
    at com.google.inject.internal.MapMaker$StrategyImpl.compute(MapMaker.java:553)
    at com.google.inject.internal.MapMaker$StrategyImpl.compute(MapMaker.java:419)
    at
com.google.inject.internal.CustomConcurrentHashMap$ComputingImpl.get(CustomConcu
rrentHashMap.java:2041)
    at com.google.inject.internal.FailableCache.get(FailableCache.java:46)
    at
com.google.inject.internal.ConstructorInjectorStore.get(ConstructorInjectorStore
.java:50)
    at
com.google.inject.internal.ConstructorBindingImpl.initialize(ConstructorBindingI
mpl.java:62)
    at com.google.inject.internal.InjectorImpl.initializeBinding(InjectorImpl.java:370)
    at
com.google.inject.internal.InjectorImpl.createJustInTimeBinding(InjectorImpl.jav
a:642)
    at
com.google.inject.internal.InjectorImpl.createJustInTimeBindingRecursive(Injecto
rImpl.java:574)
    at
com.google.inject.internal.InjectorImpl.createJustInTimeBindingRecursive(Injecto
rImpl.java:565)
    at com.google.inject.internal.InjectorImpl.getJustInTimeBinding(InjectorImpl.java:165)
    at com.google.inject.internal.InjectorImpl.getBindingOrThrow(InjectorImpl.java:125)
    at com.google.inject.internal.InjectorImpl.getInternalFactory(InjectorImpl.java:648)
    at
com.google.inject.internal.InjectorImpl.createParameterInjector(InjectorImpl.jav
a:704)
    at com.google.inject.internal.InjectorImpl.getParametersInjectors(InjectorImpl.java:692)
    at
com.google.inject.internal.ConstructorInjectorStore.createConstructor(Constructo
rInjectorStore.java:65)
    at
com.google.inject.internal.ConstructorInjectorStore.access$000(ConstructorInject
orStore.java:29)
    at
com.google.inject.internal.ConstructorInjectorStore$1.create(ConstructorInjector
Store.java:37)
    at
com.google.inject.internal.ConstructorInjectorStore$1.create(ConstructorInjector
Store.java:35)
    at com.google.inject.internal.FailableCache$1.apply(FailableCache.java:35)
    at com.google.inject.internal.MapMaker$StrategyImpl.compute(MapMaker.java:549)
    ... 40 more
Caused by: com.google.inject.internal.cglib.core.CodeGenerationException:
java.lang.reflect.InvocationTargetException-->null
    at
com.google.inject.internal.cglib.core.AbstractClassGenerator.create(AbstractClas
sGenerator.java:237)
    at
com.google.inject.internal.cglib.reflect.FastClass$Generator.create(FastClass.ja
va:64)
    at com.google.inject.internal.BytecodeGen.newFastClass(BytecodeGen.java:166)
    at
com.google.inject.internal.DefaultConstructionProxyFactory$1.<init>(DefaultConst
ructionProxyFactory.java:52)
    at
com.google.inject.internal.DefaultConstructionProxyFactory.create(DefaultConstru
ctionProxyFactory.java:50)
    at com.google.inject.internal.ProxyFactory.create(ProxyFactory.java:147)
    at
com.google.inject.internal.ConstructorInjectorStore.createConstructor(Constructo
rInjectorStore.java:82)
    at
com.google.inject.internal.ConstructorInjectorStore.access$000(ConstructorInject
orStore.java:29)
    at
com.google.inject.internal.ConstructorInjectorStore$1.create(ConstructorInjector
Store.java:37)
    at
com.google.inject.internal.ConstructorInjectorStore$1.create(ConstructorInjector
Store.java:35)
    at com.google.inject.internal.FailableCache$1.apply(FailableCache.java:35)
    at com.google.inject.internal.MapMaker$StrategyImpl.compute(MapMaker.java:549)
    ... 60 more
Caused by: java.lang.reflect.InvocationTargetException
    at sun.reflect.GeneratedMethodAccessor2.invoke(Unknown Source)
    at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
    at java.lang.reflect.Method.invoke(Method.java:597)
    at com.google.inject.internal.cglib.core.ReflectUtils.defineClass(ReflectUtils.java:384)
    at
com.google.inject.internal.cglib.core.AbstractClassGenerator.create(AbstractClas
sGenerator.java:219)
    ... 71 more
Caused by: java.lang.NoClassDefFoundError:
com/google/inject/internal/cglib/reflect/FastClass
    at java.lang.ClassLoader.defineClass1(Native Method)
    at java.lang.ClassLoader.defineClass(ClassLoader.java:620)
    ... 76 more

Is this addressed by the patch? Are there other workarounds like having guice be
loaded by the system class loader?

Thanks,
Felix

Original comment by berge...@gmail.com on 3 Aug 2009 at 6:35

GoogleCodeExporter commented 9 years ago 
Felix,

That looks like a problem I've run into when one of the classes you are trying 
to
have Guice create and inject is package-protected.  In an OSGi environment, all 
the
classes Guice is going to be creating and wiring up need to be public (including
Providers), otherwise they can't be seen from any classloader but the one 
they're
loaded from.

See http://code.google.com/docreader/#p=google-guice&s=google-guice&t=OSGi for 
more
info, particularly the "What are the constraints of using Guice's OSGi 
support?" section.

Original comment by rwallace...@gmail.com on 4 Aug 2009 at 11:39

GoogleCodeExporter commented 9 years ago 
People are seeing this in practice...

Original comment by limpbizkit on 16 Oct 2009 at 2:08

GoogleCodeExporter commented 9 years ago 
For interest, here's how I ran into it...

In Guice 1.0, I had a module that did:
  bind(Date.class);
and some code that did:
  @Inject
  Provider<Date> dateProvider;

Hence, the code can do dateProvider.get() (instead of new Date()) to get the 
current 
time.

Meanwhile, the tests inject a Provider<Date> that returns times from a Queue.

I haven't looked at the code for a while, but I think it was testing periodic 
polling 
(has the card transaction been processed yet? or have we timed out?), and 
(elsewhere) 
logging start/end times for a client session.

Original comment by ellis.m.a@gmail.com on 16 Oct 2009 at 11:00

GoogleCodeExporter commented 9 years ago 
I'm writing a container-based test at the moment to recreate this bug so it 
doesn't
re-appear. Jesse do you still want the patch trimmed down to be an "on/off" 
switch?

Or do we want to allow people to force the use of bridge class loaders? ie. some
situations you don't need an extra class loader to bridge between types and so 
we
wouldn't create one, but using an extra class loader would let you eagerly 
unload
proxy classes... (hence the 'eager' setting in the patch)

Original comment by mccu...@gmail.com on 16 Oct 2009 at 3:17

GoogleCodeExporter commented 9 years ago 
Yeah, on/off is all I want. Configurability is the enemy of simplicity, 
especially when the 
utility of the configurability isn't obvious.

Original comment by limpbizkit on 16 Oct 2009 at 4:24

GoogleCodeExporter commented 9 years ago

Original comment by mccu...@gmail.com on 31 Oct 2009 at 4:00

GoogleCodeExporter commented 9 years ago 
Updated patch to use a "class space" abstraction, which makes the code much 
clearer
and should also be a little bit faster. Also included changes to switch back to 
JDK
reflection when we can't use the CGLIB Fast* approach, which removes some of the
visibility limitations regarding OSGi (only method-intercepted classes need to 
be
public/protected with this patch).

If we do find a private method-intercepted class when running in a strict 
container
like OSGi then we now report exactly which method was involved, to help the 
user.

The other changes are minor fixes to avoid unnecessary synthetic accessors...

[NOTE: issue 443 provides extra tests that check different visibilities in OSGi]

Original comment by mccu...@gmail.com on 2 Nov 2009 at 8:09

Attachments:

GoogleCodeExporter commented 9 years ago 
Passing to Jesse for review of the latest patch.

Original comment by mccu...@gmail.com on 2 Nov 2009 at 8:10

GoogleCodeExporter commented 9 years ago 
Build of trunk with latest 343 patch applied (in case anyone wants to give it a 
spin)

Original comment by mccu...@gmail.com on 16 Nov 2009 at 4:17

Attachments:

GoogleCodeExporter commented 9 years ago 
Last patch created more bridge class loaders than absolutely necessary, because 
the
weak cache uses identity equality instead of equals. This updated patch goes 
back to
using class loaders as keys but with a special bridge class loader for system 
types.

I've also attached a build of Guice trunk with this patch applied for people to 
test.

Original comment by mccu...@gmail.com on 18 Feb 2010 at 4:59

Attachments:

GoogleCodeExporter commented 9 years ago 
guice-customloader-20100218.jar worked like a champ! thanks!

Original comment by holme...@gmail.com on 18 Mar 2010 at 9:06

GoogleCodeExporter commented 9 years ago 
Issue 417 has been merged into this issue.

Original comment by sberlin on 19 Apr 2010 at 2:36

GoogleCodeExporter commented 9 years ago 
[NOTE: I've split this patch into two parts to (hopefully) make it easier to 
digest]

Description (1of2)
==================

The attached patch (GUICE_ISSUE_343_part1of2_20100504.patch) wraps the various 
CGLIB
reflection calls in try...catch blocks so that if (for whatever reason) CGLIB 
fails
to proxy the client type we can fall back to the slower JDK reflection which 
usually
has better luck, given that it's running from the JVM zone.

There are three classes which use CGLIB to proxy client types:

A) ProxyFactory, which uses CGLIB to do method interception - there isn't an 
easy JDK
reflection fallback for this, so we do the next best thing and wrap the error 
inside
a ProvisionException. This includes a clear message indicating the failing 
class,
which is not obvious from the original cause.

B) SingleMethodInjector, which uses CGLIB's FastMethod - if this fails then we 
can
simply fall back to JDK reflection. Note that we need to make the method 
accessible
if the declaring type is not-public, not just if the method itself is not 
public.

C) DefaultConstructionProxyFactory, which uses CGLIB's FastConstructor - first 
we
need to pull the creation of the FastConstructor outside of the 
ConstructionProxy
constructor, otherwise we don't know if it will fail until it's too late. 
Second we
need to add an extra call to make the constructor accessible when falling back 
to JDK
reflection (but only if the constructor's declaring class is not public).

Original comment by mccu...@gmail.com on 4 May 2010 at 12:51

Attachments:

GoogleCodeExporter commented 9 years ago 
Description (2of2)
==================

This is a hard patch to review as it involves custom classloading, but 
hopefully the
OSGi container tests introduced in Issue 443 will increase confidence in this 
patch.
We're also using it in installations of Sonatype Nexus 1.6.0 (both standalone 
Java
and deployed as a WAR) so it's fair to say it has had a reasonable amount of 
testing
in the field.

The attached patch (GUICE_ISSUE_343_part2of2_20100504.patch) fixes a number of 
flaws
in the BytecodeGen class utility:

a) it fixes the assumption that types from the system classloader never need to 
be
bridged^ because the application must be running in that space and therefore we 
can
use the type's classloader. This assumption is incorrect: an application may be
running inside a non-system classloader and wants to proxy a system type. If 
Guice is
also running inside a different non-system classloader then it won't be visible 
from
the system classloader and bridging will still be required.

  [^ bridging is when a new classloader is used to merge two disparate classloaders
into a single view.]

b) it avoids the need for ClassLoader.getSystemClassLoader() which might not be
available on systems that are running with a locked-down security manager.

c) it only creates the weak cache if we're using custom loading (the default 
mode)

d) it handles the situation where the JVM suddenly needs access to "sun.reflect"
classes because a limit has been reached internally and it wants to redo the 
proxy

Implementation
==============

The patch can be broken down like so:

i) minor visibility changes (make class public, make certain members 
package-private
to avoid the cost of synthetic access)

ii) create a singleton bridge classloader (with initialization-on-demand so we 
only
create it when we need it) that has the system classloader as parent (ie. 
default
constructor). This way we don't need to rely on 
ClassLoader.getSystemClassLoader().

iii) *all* classloaders are canonicalized, even the ones cached inside 
BytecodeGen

iv) canonicalization maps non-null classloaders to themselves, and null 
classloaders
to the parent of our singleton bridge classloader (which may also be null). The 
key
thing is that we can test canonicalized classloaders against the singleton 
parent.

v) initialize the weak classloader cache based on the "guice.custom.loader" 
setting

vi) the following rules are used to decide if bridging is required:

  "guice.custom.loader" disabled? -> NO NEED TO BRIDGE

  same classloader as Guice? -> NO NEED TO BRIDGE

  already a bridge? -> NO NEED TO BRIDGE

  visible type and *not* from the system classloader? -> CREATE BRIDGE (cached)

  visible type and from the system classloader? -> USE SYSTEM BRIDGE

  otherwise -> UNABLE TO BRIDGE

vii) a bridge classloader is a custom classloader that has the type's 
classloader as
its parent (or system classloader in the case of the system bridge). Classes 
from
"sun.reflect" are always loaded from the system bridge using classic 
parent-first
delegation. Internal Guice classes are loaded from the Guice classloader, 
unless it's
null in which case we must be able to find them via the system bridge. A new
package-private method called "classicLoadClass" is added so we can call the 
classic
loadClass implementation (ie. parent-first delegation) between bridge 
classloaders.
This also serves as a replacement for "getParent().loadClass()" which could 
cause a
NPE if the parent classloader is null.

Original comment by mccu...@gmail.com on 4 May 2010 at 4:24

Attachments:

GoogleCodeExporter commented 9 years ago 
I have one question about part 2 of the patch.  (I'm not terribly familiar with 
all
the inner workings of classloading, so this may not be the wisest question.)

Prior to the patch, if a class's ClassLoader ('delegate') was the system class
loader, getClassLoader(Class, ClassLoader) just returned the existing 
classloader
(the system classloader).  The patch changes that so that it returns a
BridgeClassLoader whose parent is the system classloader.

This looks to be a goal of the patch (point 2), so it's clearly not a 
mistake... but
I'd like to understand the implications of it.  Will this result in objects 
whose
getClassLoader method is different than the system classloader, even if the
application doesn't use any other classloaders?  What are the implications of 
that
(if true)?  Could it result in the application having possibly having more than 
one
classloader (leading to some Classes not being equal to each other, if one was
constructed by Guice and the other manually)?

Original comment by sberlin on 8 May 2010 at 3:09

GoogleCodeExporter commented 9 years ago 
Correct, if the type's classloader is the system classloader and Guice is not 
running
in the same classloader then it will create a bridge between the system 
classloader
and the Guice classloader. Not creating a bridge for this situation is the 
cause of
the original issue, hence this change.

But this doesn't mean it always creates a bridge classloader - if Guice is also
running in the system classloader then this is detected by the following check:

   if (delegate == GUICE_CLASS_LOADER || ...

and the (canonicalized) type's classloader will be returned.

Original comment by mccu...@gmail.com on 9 May 2010 at 5:29

GoogleCodeExporter commented 9 years ago 
Also note that bridge classloaders never define any classes themselves - they 
always
delegate either to the type's classloader or the Guice classloader. So any 
non-Guice
classes loaded via the bridge classloader will be equal to the same class 
loaded by
the original type classloader (as with any parent-first delegation scheme).

Original comment by mccu...@gmail.com on 9 May 2010 at 5:52

GoogleCodeExporter commented 9 years ago 
There is one minor change we could make to reduce the number of bridge 
classloaders
even more - check to see if the type is under the "java.*" namespace. If it is 
then
we can use the Guice classloader, as this can see both Guice classes and 
"java.*"
classes. (Other non-"java.*" classes may be hidden from Guice depending on the
container's class loading model.)

Here's the extra patch to add this check:

Index: src/com/google/inject/internal/BytecodeGen.java
===================================================================
--- src/com/google/inject/internal/BytecodeGen.java     (revision 2364)
+++ src/com/google/inject/internal/BytecodeGen.java     (working copy)
@@ -128,6 +128,11 @@
       return delegate;
     }

+    // java.* types can be seen everywhere
+    if (type.getName().startsWith("java.")) {
+      return GUICE_CLASS_LOADER;
+    }
+
     delegate = canonicalize(delegate);

     // no need for a bridge if using same class loader, or it's already a bridge

Original comment by mccu...@gmail.com on 9 May 2010 at 7:28

GoogleCodeExporter commented 9 years ago 
fixed in r1158.  thanks very much for the patch, Stuart!

Original comment by sberlin on 9 May 2010 at 12:52

GoogleCodeExporter commented 9 years ago 
Hi, I am new to Guice, and currently having problems with the guice 2.0 that i 
just
downloaded.

I need the guice2.0, so that i am able to use the guicefruit for smooth jpa 
integration
( inject with @PersistenceContext ) --- i have already been able to inject ejb 
into
my struts2 action and guice is wonderful ! Now i need guice to support jpa 
injection
also.

Can you suggest me on the stable guice2.0 one ? 

Here is my stack :
com.google.inject.internal.ComputationException:
com.google.inject.internal.cglib.core.CodeGenerationException:
java.lang.reflect.InvocationTargetException-->null
        at com.google.inject.internal.MapMaker$StrategyImpl.compute(MapMaker.java:553)
        at com.google.inject.internal.MapMaker$StrategyImpl.compute(MapMaker.java:419)

Original comment by bambang....@gmail.com on 4 Jun 2010 at 6:46

GoogleCodeExporter commented 9 years ago 
BB - suggest you either compile from trunk or perhaps try the patched jar at 
http://repo2.maven.org/maven2/org/sonatype/spice/inject/guice-patches/2.1.3/

Original comment by mccu...@gmail.com on 13 Jun 2010 at 3:34