Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 312668

Summary:

[director] Only single IU Fragments are attached to IUs

Product:

[Eclipse Project] Equinox

Reporter:

DJ Houghton <dj.houghton>

Component:

Assignee:

DJ Houghton <dj.houghton>

Status:

RESOLVED FIXED

QA Contact:

Severity:

normal

Priority:

CC:

aniefer, irbull, john.arthorne, pascal

Version:

3.6

Flags:

irbull: review+
aniefer: review+
pascal: review+

Target Milestone:

3.6 RC2

Hardware:

OS:

Linux

Whiteboard:

Attachments:

Description	Flags
patch	none
alternative patch	none
alternative keeping theFragment first	none
alternative using TranslationSupport Constant	none

Description DJ Houghton

2010-05-12 12:52:39 EDT

Related to bug 311576 and bug 275274 in that we are seeing this problem in those scenarios.

In the AttachmentHelper class we have code which attaches IU Fragments to IUs. Note this does not mean bundle fragments... they are represented as regular IUs. IU fragments are (for instance) translations for the metadata or can contain touchpoint actions for installing bundles.

In the case of the bugs listed above, we have a particular product which is installing several (well over 1000) bundles via the drop-ins mechanism. As a result, if a bundle and its associated nl bundle are both discovered and installed by the drop-ins, we are creating a special translation IU Fragment. The problem is that there is already an IU Fragment on these IUs (the OSGi default one telling us they are bundles and we should call InstallBundle which adds them to the bundles.info file, etc).

The code in the AttachmentHelper class only allows a single IU fragment to be attached to an IU. The reasoning behind this is because the touchpoint data could be conflicting on the fragments and we have no way to know this beforehand. So the logic attaches the fragment with the most requirements satisfied.

In our case we are seeing the translation fragment attached to the bundle rather than the default OSGi one, and therefore the IU is being installed into the profile but the bundle is not being added to the bundles.info file. This causes the product not to load because it is missing bundles. Whether the translation fragment or the default OSGi one is attached is random. (first come, first served... stored in a Set so based on VM, etc)

We discussed this problem and potential solutions. We cannot attach all fragments because of the potential touchpoint data conflict problem.

I will attach a patch which takes the approach of:
- retaining the current logic of attaching the fragment with the most satisfied requirements
- always attaching all the translation fragments

Since translation fragments are 1) known by the p2 core and 2) do not contain touchpoint data which can conflict with other fragments, we feel that this is a relatively safe fix. The IU fragment attachment problem in general still needs to be reviewed in the future but this is a temporary fix for the 3.6 release.

Comment 1 Andrew Niefer

2010-05-12 13:13:06 EDT

(In reply to comment #0)
> I will attach a patch which takes the approach of:
> - retaining the current logic of attaching the fragment with the most satisfied
> requirements
> - always attaching all the translation fragments

Just to clarify, the current logic of attaching the fragment with the most satisfied requirement can potentially select a translation fragment.  So, don't we really mean to modify the current logic a bit to make this selection from the non-translation fragments.

Comment 2 DJ Houghton

2010-05-12 16:52:53 EDT

Created attachment 168264 [details]
patch

Initial patch.

There is a bit of weird logic here because we are trying to keep the first element in the array to be the originally selected non-translation fragment.

Andrew: yes we are removing the translation fragments from the selection process so they are never chosen and are always added at the end.

Comment 3 DJ Houghton

2010-05-13 10:41:08 EDT

Pascal, what are your thoughts on this approach?

Comment 4 DJ Houghton

2010-05-13 13:54:42 EDT

Created attachment 168428 [details]
alternative patch

Here is an alternative patch if we are not concerned about the non-translation fragment always being the first element in the collection.

Comment 5 Andrew Niefer

2010-05-13 16:00:43 EDT

Created attachment 168464 [details]
alternative keeping theFragment first

Here is an alternative to the original which keeps the selected fragment as the first one by using LinkedList instead of ArrayList.

Comment 6 Ian Bull

2010-05-17 01:04:22 EDT

Created attachment 168673 [details]
alternative using TranslationSupport Constant

This builds on Andrews patch and uses the TranslationSupport constant for the localization namespace. Note, I did have to change a field from package visibility to public (non API field), so this might not be a good idea. 

I'm giving a +1 to andrews patch (I think we should keep the non-translation fragments at the head of the list since they more important, and while I doubt anybody depends on this, you never know).  

If you think that we should use the localization constant, then you can apply my patch, otherwise we can go with Andrew's.

Comment 7 DJ Houghton

2010-05-17 10:01:38 EDT

I vote for Andrew's patch and then a TODO to change the code and use the constant in 3.7.

Comment 8 DJ Houghton

2010-05-17 10:34:11 EDT

Pascal, if you have reservations about these changes please comment. We can also talk about this at the p2 call today.

Comment 9 Pascal Rapicault

2010-05-18 10:43:15 EDT

The patch looks safe. However as discussed in yesterday's call we need to understand what has changed since 3.5. Is the metadata different? 
Also adding a few tests would be good.

Comment 10 DJ Houghton

2010-05-18 12:11:19 EDT

I haven't looked at the behaviour in 3.5.2 but the reason we don't see the problem in 3.4.2 is because the code has changed a lot and we aren't following the same code paths. Although the actual code to attach fragments and keep only one remains the same (second part of ResolutionHelper#attachCUs), the context under which we are evaluating the IUs and fragments has changed dramatically. The code has been re-written and no longer uses the OSGi resolver to resolve the state as well there are generator/publisher changes.

I am able to reproduce this problem consistently on 3.6 with some debugged/tweaking. Setup and reproducibility is VM dependent. (noted below)
- have a bundle and its nl fragment both in the dropins and start
- set a breakpoint in DirectoryWatcher#scanDirectories and when you get the file list back, ensure that the bundle is the first element and the nl fragment is the second (VM dependency issue #1)
- set a breakpoint in AttachmentHelper and when we build up the iusToFragment list, you will see that there are multiple potential fragments for your new bundle. (the osgi default and the localization one)
- depending on which fragment is first in the list, that is the one which will be attached. the list is actually a Set so the order depends on hash. (VM dependency issue #2)
- if the localization fragment was first in the list then the bundle would not be installed correctly. if the osgi tooling fragment was first in the list then the bundle would be missing the localization fragment attachment.

Comment 11 DJ Houghton

2010-05-18 12:15:18 EDT

I have released Andrew's patch with a comment about using the constant in the TranslationSupport class in the next release.