BytecodeGen uses system classloader when applying AOP to system types #343
Comments
|
From mcculls on March 03, 2009 05:15:17 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. |
|
From mcculls on March 03, 2009 06:57:47 Suggested patch along with an update to the BytecodeGen testcases. Because we now use a bridging classloader when the type classloader is different to Attachment: gist |
|
From mcculls on March 03, 2009 07:35:58 This is an enhanced patch that adds the ability to turn on bridge classloading even To turn on eager bridge classloading use: -Dguice.custom.loader=eager Attachment: gist |
|
From mcculls on March 03, 2009 08:38:18 FYI, this is the original thread discussing the problem: http://groups.google.com/group/guice-osgi/browse_thread/thread/a1030d7500a29dec |
|
From mcculls on March 03, 2009 09:40:19 Latest patch that also wraps the call to getSystemClassLoader in a doPrivileged block. Attachment: gist |
|
From mcculls on March 30, 2009 03:57:57 I'd like to get this patch into Guice 2.1 if possible. Labels: Milestone-Release2.1 |
|
From limpbizkit on April 01, 2009 23:25:50 I'll patch this in. Labels: -Milestone-Release2.1 Milestone-Release2.0 |
|
From limpbizkit on April 26, 2009 14:36:15 (No comment was entered for this change.) Labels: Priority-Low |
|
From limpbizkit on May 08, 2009 21:34:34 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 Could you write a test? I'm tempted to tidy up the code, but I don't want to break anything! Status: Started |
|
From mcculls on May 10, 2009 05:17:44 Originally we supported just two modes: <unset>/"true" - only use bridge classloaders where needed (and possible) 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 Now this is really a "nice-to-have", so if you think it will confuse people we can The other change is to support situations where we can't get the system classloader Feel free to tidy up the patch while I write up a test for this change - you can |
|
From limpbizkit on May 13, 2009 00:38:55 I'd like to make things work with only the two modes. I'll try to tweak your patch to handle this. |
|
From mcculls on May 21, 2009 02:02:43 This patch didn't make it into 2.0, so rescheduling it for 2.1 Labels: -Milestone-Release2.0 Milestone-Release2.1 |
|
From bergerfx on August 03, 2009 11:35:06 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 I'm running into the following exception: SEVERE: Application class org.limewire.ui.swing.mainframe.AppFrame failed to launch
1 error |
|
From bergerfx on August 03, 2009 11:35:06 ...oogle.inject.internal.cglib.core.CodeGenerationException: Is this addressed by the patch? Are there other workarounds like having guice be Thanks, |
|
From rwallace1979 on August 04, 2009 16:39:43 Felix, That looks like a problem I've run into when one of the classes you are trying to See https://code.google.com/docreader/#p=google-guice&s=google-guice&t=OSGi for more |
|
From limpbizkit on October 15, 2009 19:08:24 People are seeing this in practice... Labels: -Priority-Low Priority-High |
|
From ellis.m.a on October 16, 2009 04:00:32 For interest, here's how I ran into it... In Guice 1.0, I had a module that did: Hence, the code can do dateProvider.get() (instead of new Date()) to get the current 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 |
|
From mcculls on October 16, 2009 08:17:37 I'm writing a container-based test at the moment to recreate this bug so it doesn't Or do we want to allow people to force the use of bridge class loaders? ie. some |
|
From limpbizkit on October 16, 2009 09:24:29 Yeah, on/off is all I want. Configurability is the enemy of simplicity, especially when the |
|
From mcculls on October 30, 2009 21:00:48 (No comment was entered for this change.) Owner: mcculls |
|
From mcculls on November 02, 2009 00:09:04 Updated patch to use a "class space" abstraction, which makes the code much clearer If we do find a private method-intercepted class when running in a strict container The other changes are minor fixes to avoid unnecessary synthetic accessors... [NOTE: issue 443 provides extra tests that check different visibilities in OSGi] Attachment: gist |
|
From mcculls on November 02, 2009 00:10:42 Passing to Jesse for review of the latest patch. Owner: limpbizkit |
|
From mcculls on November 16, 2009 08:17:35 Build of trunk with latest 343 patch applied (in case anyone wants to give it a spin) Binary attachments: guice-customloader-20091116.jar |
|
From mcculls on February 18, 2010 08:59:18 Last patch created more bridge class loaders than absolutely necessary, because the I've also attached a build of Guice trunk with this patch applied for people to test. Attachment: gist |
|
From holmes.j on March 18, 2010 14:06:54 guice-customloader-20100218.jar worked like a champ! thanks! |
|
From sberlin on April 19, 2010 07:36:52 Issue 417 has been merged into this issue. |
|
From mcculls on May 04, 2010 05:51:42 [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 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 B) SingleMethodInjector, which uses CGLIB's FastMethod - if this fails then we can C) DefaultConstructionProxyFactory, which uses CGLIB's FastConstructor - first we Owner: sberlin Attachment: gist |
|
From mcculls on May 04, 2010 09:24:04 Description (2of2)This is a hard patch to review as it involves custom classloading, but hopefully the The attached patch (GUICE_ISSUE_343_part2of2_20100504.patch) fixes a number of flaws a) it fixes the assumption that types from the system classloader never need to be [^ brid] b) it avoids the need for ClassLoader.getSystemClassLoader() which might not be 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" ImplementationThe patch can be broken down like so: i) minor visibility changes (make class public, make certain members package-private ii) create a singleton bridge classloader (with initialization-on-demand so we only iii) all classloaders are canonicalized, even the ones cached inside BytecodeGen iv) canonicalization maps non-null classloaders to themselves, and null classloaders 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 Attachment: gist |
|
From sberlin on May 08, 2010 08:09:21 I have one question about part 2 of the patch. (I'm not terribly familiar with all Prior to the patch, if a class's ClassLoader ('delegate') was the system class This looks to be a goal of the patch (point 2), so it's clearly not a mistake... but |
|
From mcculls on May 08, 2010 22:29:34 Correct, if the type's classloader is the system classloader and Guice is not running But this doesn't mean it always creates a bridge classloader - if Guice is also if (delegate == GUICE_CLASS_LOADER || ... and the (canonicalized) type's classloader will be returned. |
|
From mcculls on May 08, 2010 22:52:25 Also note that bridge classloaders never define any classes themselves - they always |
|
From mcculls on May 09, 2010 00:28:17 There is one minor change we could make to reduce the number of bridge classloaders Here's the extra patch to add this check: Index: src/com/google/inject/internal/BytecodeGen.java + // java.* types can be seen everywhere // no need for a bridge if using same class loader, or it's already a bridge |
|
From sberlin on May 09, 2010 05:52:40 fixed in r1158 . thanks very much for the patch, Stuart! Status: Fixed |
|
From bambang.teleinfocomproject on June 03, 2010 23:46:06 Hi, I am new to Guice, and currently having problems with the guice 2.0 that i just I need the guice2.0, so that i am able to use the guicefruit for smooth jpa integration Can you suggest me on the stable guice2.0 one ? Here is my stack : |
|
From mcculls on June 12, 2010 20:34:16 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/ |
From mcculls on March 03, 2009 08:06:15
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: http://code.google.com/p/google-guice/issues/detail?id=343
The text was updated successfully, but these errors were encountered: