Bug 332504

Comment 1 Dani Megert 2010-12-14 05:37:04 EST

*** Bug 332505 has been marked as a duplicate of this bug. ***

Comment 2 Stefan Röck 2010-12-14 05:58:07 EST

Created attachment 185119 [details]
Proposed Patch

Iterate twice over the newTargets. First remove all dependencies that would lead to a dependency loop, second compare with existing dependencies in target list.

Comment 3 Ovidio Mallo 2010-12-14 16:50:01 EST

Stefan, thanks for the patch! Would you mind doing the following changes to your patch in order to see whether we can apply it:
* include a contribution note to the header of all the files you touched (including your name and the bug number as in the existing ones)
* please move the (existing) comment "This loop is roughly equivalent to..." in the patch from the first to the second loop where it fits better
* please try to write a test in MultiValidatorTest reproducing the stack overflow which should be fixed by your patch

BTW, if you need a workaround for the bug, you might try accessing the targets observable inside the validate method protected by a "ObservableTracker.setIgnore(true);", something like the following:

  ObservableTracker.setIgnore(true);
  try {
    // observables accessed here are not tracked as dependencies
  } finally {
    ObservableTracker.setIgnore(false);
  }

Comment 4 Stefan Röck 2010-12-16 03:00:19 EST

Created attachment 185295 [details]
Improved patch

Ovidio, I changed point 1 and 2 according to your suggestions. Unfortunately, I didn't manage to implement a test case that determistically fails. The reason is that the dependency array returned by the ObservableTracker is based on a HashSet. As the order is crucial (as described in first comment) all my attempts only sometimes fail - which is obviously bad. Maybe you have an idea for a predictable test case?

Comment 5 Ovidio Mallo 2010-12-21 14:41:46 EST

Stefan, indeed it's probably hard to make the test causing a stack overflow deterministic since ObservableTracker uses an IdentitySet which uses System.identityHashCode(Object) which can change on each run of the test.

However, you might test the root cause of the problem which is the fact that the internal observables end up in the "targets" list. You might write a test which accesses all of the internal observables inside the validate() method and make sure that none of them ends up in the "targets" list.

Comment 6 Matthew Hall 2011-09-18 02:07:43 EDT

I think you hit the nail on the head: we filter out the new dependencies while iterating over the old ones, we can get internal observables past the filter by referencing them where there are no dependencies.

I'm working on a test case for this.

Comment 7 Matthew Hall 2011-09-19 02:12:28 EDT

Created attachment 203567 [details]
Patch with tests

This patch includes Stefan's fix plus a unit test for it.

It also includes a fix plus test for an infinite loop in the getValidationStatus lazy init section.

Stefan, can you test whether this patch addresses your problem?

Comment 8 Matthew Hall 2011-09-19 02:12:33 EDT

Created attachment 203568 [details]
mylyn/context/zip

Comment 9 Matthew Hall 2011-09-19 02:13:14 EDT

comment 7: s/infinite loop/infinite recursion/

Comment 10 Matthew Hall 2011-09-19 02:14:15 EDT

Ovidio, +1?

Comment 11 Ovidio Mallo 2011-09-19 17:13:50 EDT

+1

Comment 12 Matthew Hall 2011-09-19 23:29:20 EDT

Released to HEAD > 20110919

Comment 13 Matthew Hall 2011-09-22 20:56:43 EDT

This should have been committed to git. Reopening.

Comment 14 Matthew Hall 2011-09-23 02:03:21 EDT

Committed changeset c5f5b7d8201363f4bbe7cdb60f8de9ce51709d13 to R3_maintenance > 2011-09-23

Comment 15 Dani Megert 2012-08-24 05:21:41 EDT

Just for the records: the bundle version was not set correctly: it should have been 1.4.100.