search

gwtproject / gwt

GWT Open Source Project
http://www.gwtproject.org
1.53k stars 378 forks source link

Deserialization cycling (freeze) caused by object hierarchy with generics #7776

Closed dankurka closed 9 years ago

dankurka commented 9 years ago

Originally reported on Google Code with ID 7779

Found in GWT Release (e.g. 2.4.0, 2.5.0 RC):
2.5.0-rc1, 2.5.0-rc2

Encountered on OS / Browser (e.g. WinXP, IE8-9, FF7):
Google Chrome 22.0.1229.94 m (probably it's not connected with specific browser)

Detailed description (please be as specific as possible):

RPC deserialization doesn't finish if we deserialize object with class definition containting
generics and implementing interface uses the same generics. That object has to contain
next object implementing the same interface with generics (as value in property).

Example:
public class Person<PERSON_CAR extends Car> implements Being<PERSON_CAR>, Serializable
{
    private Being<PERSON_CAR> child;
...
}

During debugging we've found that problem appears in cycle of method "SerializabilityUtil.findActualType()".
We've attached screenshot (freeze.png) with problematic cyclic reference.

We've also attached simple demonstration project (CriterionTest.zip, CriterionTest.war).

Shortest code snippet which demonstrates issue (please indicate where
actual result differs from expected result):

1) interface:
public interface Being<BEING_CAR extends Car> {

}

2) object class:
public class Person<PERSON_CAR extends Car> implements Being<PERSON_CAR>, Serializable
{

    private Being<PERSON_CAR> child;

    public Person() {
    }

    public void setChild(Person<PERSON_CAR> child) {
        this.child = child;
    }
}

3) RPC method:
void load(Being<METHODCAR> being);

4) example of calling when stuck appears:
ControllerAsync.Pool.get().load(person, new AsyncCallback<String>() {...}
);

Workaround if you have one:

We don't have workaround, but we discovered that in "resolvedTypes" map there are more
values for one key. If we understand it correctly, now only first value is used. We
think that last one should be used. Something like (SerializabilityUtil:237):
result = resolvedTypes.map.get((TypeVariable<?>) result).getLast()

Links to relevant GWT Developer Forum posts:

Link to patch posted at http://gwt-code-reviews.appspot.com:

Reported by gola@effectiva.cz on 2012-11-09 14:30:37

dankurka commented 9 years ago 
One more important thing - it works without any problems in GWT 2.4.0.

Reported by gola@effectiva.cz on 2012-11-12 10:47:08

dankurka commented 9 years ago 
No reaction? It's really critical for us. We can help with fix, but we need support.

Reported by gola@effectiva.cz on 2012-11-21 08:30:16

dankurka commented 9 years ago 
@skybrian: you made some changes to the SerializableTypeOracleBuilder recently, so you
might have an idea of what's going on here.

Reported by t.broyer on 2012-11-21 11:04:38

dankurka commented 9 years ago 
I couldn't find any solid solution to this, but I did manage to narrow the search down
to r10582 https://code.google.com/p/google-web-toolkit/source/diff?spec=svn10582&r=10582&format=side&path=/trunk/user/src/com/google/gwt/user/server/rpc/impl/SerializabilityUtil.java

The type prechecker code was introduced in 10518, but it didn't break until the next
change to the rpc code in 10582.

The comment around the infinite loop (since 10518) is:
    // Look for things that TypeVariables are mapped to, but stop if mapped
    // to itself. We map a TypeVariable to itself when we wish to explicitly
    // mark it as unmapped.

The main problem is that the concrete class Person is bound to Being and Being is bound
to Person, so the type check loop just grinds between the two.  It may be worthwhile
to also iterate through TypeVariable.getBounds() to check for equality there, though
I'm not sure how that would affect interfaces that are bound to multiple types...

Also worth noting, the interface Being<> is not included anywhere in the .rpc files,
and the .rpc files do not change across the good and bad gwt revisions.  It is bombing
on the field that is interface type Being<>; changing it to Person<> no longer causes
the loop (as Person correctly maps to itself).

I've tried adding new types that implement Being to the compile, and it doesn't seem
to make any difference at all; somewhere in SerializabilityUtil.resolveTypesWorker(),
the interface field is mapped to the concrete type Person, while the concrete type
Person is mapped to the interface Being...

Reported by James@wetheinter.net on 2012-12-11 05:24:25

dankurka commented 9 years ago 
Okay,

So, a few more hours of hacking, and I've found a workable, albeit imperfect solution.

The problem revolves around the generic type signatures for the interface field causing
a cyclic dependency error where the generic for Being<Car> is resolved first, to BEING_CAR=METHODCAR,
then Person<Car> is resolved to PERSON_CAR=BEING_CAR.  Then, when the type resolver
descends into the fields, it sees Being<Car>, and creates a mapping BEING_CAR=[PERSON_CAR,
METHODCAR].  Then, when it tries to call lookupActualType(), it hits the infinte loop
because the resolvedTypeMap looks like:
{PERSON_CAR=[BEING_CAR], BEING_CAR=[PERSON_CAR, METHODCAR]}

The previous implementation did not consider this type of cyclic dependency, and, in
all likelihood, probably shouldn't.  BEING_CAR should probably resolve to the same
generic type as Person<PERSON_CAR extends Car>, as that is where the one and only one
definition of the actual generic type exists in the class.  However, because this resolving
is done natively by the jvm, I'm not sure it's in our power to change this behavior.

All I did to break the cycle was to change
-    while (result instanceof TypeVariable<?> &&
-        resolvedTypes.get((TypeVariable<?>) result) != result &&
-        resolvedTypes.get((TypeVariable<?>) result) != null) {
-      result = resolvedTypes.get((TypeVariable<?>) result);

to

+    while (result instanceof TypeVariable<?>) {
+      Type resolvedType = resolvedTypes.get((TypeVariable<?>) result);
+      // This type maps to itself and cannot be resolved at this time
+      if (resolvedType == null || resolvedType == result) {
+        return result;
+      }
// here we look backwards instead of just forwards to detect the loop.
+      if (result == resolvedTypes.get((TypeVariable<?>)resolvedType)) {
+        return resolvedType;
+      }
+      // continue searching
+      result = resolvedType;

Looking at how this method is used, this >appears< safe, as the default action for
types that cannot be resolved directly is to simply return the TypeVariable for the
method to keep searching in other places.  I prefered returning the resolvedType instead
of result when the loop is detected, as this pointed to PERSON_CAR rather than BEING_CAR,
and PERSON_CAR is the generic from the class definition itself.

A better solution would be to prevent this incorrect mapping in the first place, but
that goes beyond my skill and amount of free time.  

Also note, the comment on the breaking commit:

Addressing a problem with GWT RPC type checking failing to correctly
detect the expected type when the object that is passed as an argument
is actually a base class of the expected type. In such cases, the
previous, wrong behavior was to use the instance's type variables to
resolve the expected type. The fix is to resolve the types using the
instance "cast" to the expected type.

This changes the method signatures for custom serializers. Users will
need to modify their code if they have already produced server-side
custom serializers.

---So, it appears the fix was for passing a base class through rpc, and our problem
here is from a superinterface having its generic type's source blown away by allowing
the jvm to assign different generic types to the same type argument.

We should likely, at the very least, update RPCTypeCheckTest to include a class hierachy
matching gola's use case:
class SomeType<T extends Foo> implement IFace<T>{
  IFace<T> field;
}

@gola - If you need an immediate fix, the patch I've attached will work against trunk,
though I can't promise you it won't have undesired consequences; I'm not setup to run
the full test suite, so all I can tell you is this will fix your problem, and >probably<
won't cause new problems.

If you want me to send you compiled jars, just let me know, and I can upload them somewhere
for you; you'll need to use <scope>system</scope> <systemPath>/absolute/path/to/jars</systemPath>
in both your project dependencies, and in gwt-maven-plugin (or you can overwrite the
copy in your repo, though they'll fail checksum and probably get overwritten by maven
-U).  

Gwt-2.5.1 >should< be getting released by the end of the month, with a proper fix for
your issue.

Reported by James@wetheinter.net on 2012-12-11 09:18:36

dankurka commented 9 years ago 
Forgot to attach the diff, since I browsed away from the page to get gola's username.

If you want me to open a code review, lemme know; I'm not exactly experienced in rietveld,
but for a change this small, I figured a patch was good enough.

Reported by James@wetheinter.net on 2012-12-11 09:20:38

dankurka commented 9 years ago 
Better switch to Gerrit already ;-)
See https://groups.google.com/d/msg/google-web-toolkit-contributors/fmHDlsnfdEQ/fc6lvNdxROQJ
to get started.
And try to add a unittest.

Reported by t.broyer on 2012-12-11 09:57:09

dankurka commented 9 years ago 
If you can point me at some directions for launching the builtin test cases, I can add
the use case to RPCTypeCheckTest where it belongs (and also test all the other generic
type-dependent tests as well).

I started running ant test, but it was a) painfully slow, and b) failed a lot due to
missing dependencies, I'm guessing.

I also tried including user/test and dev/test as source roots, but that only served
to lock up eclipse and waste twenty minutes of my night. :)

Reported by James@wetheinter.net on 2012-12-11 11:49:43

dankurka commented 9 years ago 
Using the projects in the eclipse/ folder of the sources (and following the README to
set them up), you should be able to run the tests right from Eclipse.

From the command line: after an "ant dist-dev", something like "cd user && ant test.nongwt
-Dgwt.nongwt.testcase.includes=**/RPCTypeCheckTest.class"
If you need some GWTTestCase, then "ant test.dev.htmlunit -Dgwt.junit.testcase.includes=**/RPCTypeCheckTest.class"
(or test.web.htmlunit to run them in prod mode)

BTW, if you use Git, you'll need -Dgwt.svnrev=trunk@12345 for each Ant invocation (the
12345 represents the SVN trunk revision, but it doesn't really matter; it's used for
some book-keeping, so just do an "ant clean" from time to time and otherwise you can
use whichever value you want)

Reported by t.broyer on 2012-12-11 13:39:21

dankurka commented 9 years ago 
Awesome; thanks a bunch.  I ran various ant related errors that didn't seem to be worth
the trouble atm, but that direct testing was very useful; I passed my test, but broke
six others ^_^

Anyway, I have it down to just one test failing, but it's on code I didn't touch in
any way...  If you or anyone else reading along gets a chance to, can you run the .nongwt
test using the cli command you gave me, and let me know if you get one failure on RPCTypeCheckTest.testValueSpoofing?
 I reset to HEAD and still got the fail, so it seems to be a legit break not caused
by my meddling.

Also, if build/ isn't already added to .gitignore, it definitely should be ;-}

Reported by James@wetheinter.net on 2012-12-11 14:08:41

dankurka commented 9 years ago 
@gola - Through my testing, I've also discovered another quick fix to your problem...
 If you use Person<> in your rpc call instead of Being<>, it will probably run just
fine.  ...A unit test that doesn't fail when it's supposed to isn't much of a test
at all.

I'm going to push this for review despite the failure I didn't cause; the test case
reproduces the problem (if I remove the fix and keep the test), so once the gwt team
clears it, you should be able to pull it in when 2.5.1 lands.

The only concern now is that if there is ever a regression, it will lockup the test
case until it times out...  which is what... an hour? ^_^  Should I include a timeout
killswitch of some kind?  I miss @Test(timeout=), but jat's rpc boilerplate is too
good to pass up!

Reported by James@wetheinter.net on 2012-12-11 14:40:27

dankurka commented 9 years ago 
Review @ https://gwt-review.googlesource.com/1410

Reported by James@wetheinter.net on 2012-12-11 15:19:23

dankurka commented 9 years ago

Reported by t.broyer on 2012-12-11 15:50:48

dankurka commented 9 years ago

Reported by kurka.daniel on 2013-01-11 21:56:46

dankurka commented 9 years ago 
This issue was closed by revision r11467.

Reported by gwt.mirrorbot on 2013-01-23 00:46:53

dankurka commented 9 years ago

Reported by skybrian@google.com on 2013-01-23 01:04:50

dankurka commented 9 years ago 
bulk edit: setting fixed issues for GWT 2.5.1 to Fixed

Reported by kurka.daniel on 2013-03-12 12:17:45