Throw exception when last candidate is removed and mandatory requirement

[laeubi]

Currently it can happen that the resolver is removing a candidate but it is the last possible one to select. If the requirement is not optional this lead guaranteed to an unresolvable state and must be handled already on the call site.

This adds throwing a runtime exception for this case to make sure such illegal modifications do not goes unnoticed and we can fix such places.

Only a draft as it would be currently too dangerous to be merged but hopefully will fail some tests to further analyze the problem.

[github-actions]

Test Results

  807 files  ±0    807 suites  ±0   1h 19m 4s ⏱️ + 4m 29s
2 201 tests +1  2 149 ✅  - 2   49 💤 ±0  0 ❌ ±0  3 🔥 +3 
6 615 runs  +3  6 457 ✅  - 6  149 💤 ±0  0 ❌ ±0  9 🔥 +9 

For more details on these errors, see this check.

Results for commit 9c36d0c. ± Comparison against base commit 4c55069.

♻️ This comment has been updated with latest results.

[laeubi]

Great (or not so great of course) we now have two test-cases failing here showing the problem:

• AutomatedTests org.eclipse.osgi.tests.container.AllTests org.eclipse.osgi.tests.container.TestModuleContainer.testStaticSubstituted
• AutomatedTests org.eclipse.osgi.tests.container.AllTests org.eclipse.osgi.tests.container.TestModuleContainer.testLocalUseConstraintViolations

that's really nasty as it blows up the search space by evicting valid solutions and instead running in circles to find alternatives!

[laeubi]

I'm now able to reproduce the problem with the reduction provided here

• Drop invalid package providers when adding a permutation #1086

and only three bundles (even though the error message seems wrong, or it shows that we are removing wrong things).

[laeubi]

I was now able to even further reduce the testcase and it is now quite minimal.

[laeubi]

I think I now understand what is happening here, we start for the testcase with this rather simple setup

[ aQute.libg ]
  [?]aQute.libg 7.1.0.202411251545 (UNRESOLVED))
    [?]Import-Package: aQute.bnd.exceptions; version="[3.0.0,4.0.0)": 
        Export-Package: aQute.bnd.exceptions; bundle-symbolic-name="aQute.libg"; bundle-version="7.1.0.202411251545"; version="3.0.0"
        Export-Package: aQute.bnd.exceptions; bundle-symbolic-name="biz.aQute.bnd.util"; bundle-version="7.1.0.202411251545"; version="3.0.0"

[ biz.aQute.bnd.util ]
  [?]biz.aQute.bnd.util 7.1.0.202411251545 (UNRESOLVED))
    [?]Import-Package: aQute.bnd.exceptions; version="[3.0.0,4.0.0)": 
        Export-Package: aQute.bnd.exceptions; bundle-symbolic-name="aQute.libg"; bundle-version="7.1.0.202411251545"; version="3.0.0"
        Export-Package: aQute.bnd.exceptions; bundle-symbolic-name="biz.aQute.bnd.util"; bundle-version="7.1.0.202411251545"; version="3.0.0"
        
[ biz.aQute.bndlib ]        
  [?]biz.aQute.bndlib 7.1.0.202411251545 (UNRESOLVED))
    [?]Import-Package: aQute.bnd.exceptions; version="[3.0.0,4.0.0)": 
        Export-Package: aQute.bnd.exceptions; bundle-symbolic-name="aQute.libg"; bundle-version="7.1.0.202411251545"; version="3.0.0"
        Export-Package: aQute.bnd.exceptions; bundle-symbolic-name="biz.aQute.bnd.util"; bundle-version="7.1.0.202411251545"; version="3.0.0"
    [!]Import-Package: aQute.bnd.result; version="[2.0.0,3.0.0)": 
        Export-Package: aQute.bnd.result; bundle-symbolic-name="biz.aQute.bnd.util"; bundle-version="7.1.0.202411251545"; version="2.0.0"; uses:="aQute.bnd.exceptions"

Because of the uses constraint we can reduce biz.aQute.bndlib to the following (and it becomes fully defined as a result of this):

[ Reduced biz.aQute.bndlib ]
  [!]biz.aQute.bndlib 7.1.0.202411251545 (UNRESOLVED))
    [!]Import-Package: aQute.bnd.exceptions; version="[3.0.0,4.0.0)": 
        Export-Package: aQute.bnd.exceptions; bundle-symbolic-name="biz.aQute.bnd.util"; bundle-version="7.1.0.202411251545"; version="3.0.0"
    [!]Import-Package: aQute.bnd.result; version="[2.0.0,3.0.0)": 
        Export-Package: aQute.bnd.result; bundle-symbolic-name="biz.aQute.bnd.util"; bundle-version="7.1.0.202411251545"; version="2.0.0"; uses:="aQute.bnd.exceptions"

this is because bndlib import package aQute.bnd.result that has only a single provider that has a uses constraint on aQute.bnd.exceptions and therefore we must throw away the aQute.libg provider of package aQute.bnd.exceptions.

Now the resolver begins and come to the conclusion that aQute.libg has a substitution package and decides to resolve it using the EXTERNAL rule that means it tries to discard from all dependents until it finds its own export, but when like in this case, the provider aQute.libg was already dropped, this means it will effectively drop everything until nothing is there anymore and then the resource biz.aQute.bndlib will not have any providers left and is considered unresolved.

So I think this assumption taken here is only ever valid in special cases where the provider of the package was not previously ruled out, I also don't see why we should remove any higher ranked providers ... so this part must be changed to ignore/remove only the one provider regardless of where it is coming in the list!
This is currently not really possible due to the data-model.

[laeubi]

While working on this I came to the conclusion that there is another case where the Bundle A has package X and it uses this initial ordering:

• X
  • B
  • A (our own candidate)
  • C

So while we have selected B for X the export must be discarded. When we choose A for X we need to keep it and then for C we need to again discard the export. It therefore seems necessary to be able to determine the substitution state we not need only two permutations but three. But one could combine all cases where the export is dropped, its just not 100% clear to me if we then still retain priority order and it also mean we need to perform this once globally what would result in much candidate objects to be created.

[laeubi]

This now has lead me back to org.apache.felix.resolver.Candidates.canRemoveCandidate(Requirement) what also previously have lead to some problems but I didn't wanted to touch it back then. So if we can solve the permutation problem this method is likely not needed anymore together with all the lockups associated with substitution packages.

My rough idea here would be as follows:

• Whenever we pop a permutation from the stack we call a method to resolve substitution-packages
• For this to work, we need a list of all CandidateSelectors that are substitution packages (that is similar to what org.apache.felix.resolver.Candidates.populateSubstitutables() currently does)
• We then need to iterate over all substitution packages and do the following
  • If there are more than one candidate we need to create a permutation to not loose any alternative and add it to the stack
  • purge all other providers from the list to make this unique and prevent it from being further permutated (as we already have put such permutation on the stack in case there are more than one).
  • If the current candidate resolves to the same resource as the requirement we have the Internal case and are done
  • Otherwise we need to purge the Provider form all other resources list of alternatives to account for the External case.

This then has the advantage that many things become unique and we can even rule out other alternatives. My plan would be to make this extracted into an own class, so we can have an emergency switch in case something goes really wrong after the release as is is a quite fundamental change.

[laeubi]

In this example we see the following:

• libg has two possible choices for its substitution package, the internal one is chosen in first iteration -> resolved
• util has two possible choices for its substitution package, the external one is chosen in first iteration -> libg
• now util has to be removed as a provider only having libg as the only one left
• bndlib now can only use libg for exceptions package but this conflicts with result from util that has use constraint on exceptions package
• on second iteration now libg chose external and drops it exports removing it from util+bndlib -> resolved state

As the situation is known to be unsolvable we can actually maybe drop all permutations that have more failed resources than others currently in the list to cut down the work for the resolver. We just need to keep one in case there is no solution at all.

[laeubi]

Some improvements has already been merged and the testcase is added as part of:

• Add two new testcases and assert the uses permutations added #1119