Skip to content

avoids IllegalArgumentException on unget #334

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

juergen-albert
Copy link
Contributor

fixes https://issues.apache.org/jira/browse/FELIX-6726

ungetServiceObject may be called twice in some cases, which causes an
IllegalArgumentException to be thrown and logged. It furthermore causes
to recreated ComponentServiceObjects to be created and never cleaned up
afterwards.

@juergen-albert
Copy link
Contributor Author

@tjwatson It would be great if you could have a look at this.

fixes https://issues.apache.org/jira/browse/FELIX-6726

ungetServiceObject may be called twice in some cases, which causes an
IllegalArgumentException to be thrown and logged.

Signed-off-by: Juergen Albert <[email protected]>
@juergen-albert juergen-albert changed the title avoids IllegalArgumentException and memory hoarding avoids IllegalArgumentException Sep 11, 2024
@juergen-albert juergen-albert changed the title avoids IllegalArgumentException avoids IllegalArgumentException on unget Sep 11, 2024
@tjwatson
Copy link
Member

tjwatson commented Jun 23, 2025

The prototype service references were implemented before my involvement in the SCR implementation.  I admit that I am unclear on exactly how the prototype references are supposed to be handled, but I think there is a larger issue with how the prototype references are managed and tracked beyond this IllegalArgumentException.

Consider a component that has two field references that are prototype required.  In this case lets have them be a reference to A and a reference to B:

A reference1;
B reference2;

Should reference1 and reference2 here be the SAME object or should SCR cause the prototype service to create two instances of BImpl

Right now it appears Felix SCR creates only one BImpl instance and injects that same object to both reference1 and reference2. But from the specification I am unclear if that is the right behavior. My expectation is that we would have to instances of BImpl tracked and injected into the two fields.

The other problem, which is more directly related to this issue, is that this call ends up ungetting the service object:

DependencyManager.invokeUnbindMethod(ComponentContextImpl<S>, RefPair<S, T>, int, EdgeInfo)

In fact it ends up ungetting all prototype instances from the prototye factory and closing down the tracking. This seems wrong to me and I am unclear what would happen if we had reference1 and reference2 above but made reference2 a dynamic/optional reference and changed the service properties such that reference2 become unsatisfied but reference1 remained valid. my guess is that SCR will unget the service object for reference2, but that would be wrong since reference1 still would reference the same object.

With all that said, this appears to be a much more complicated issue that I cannot spend time one for now and I am not convinced the fix in the current PR is correct for non-prototype service references. For now I suggest we catch the IllegalArgumentException in org.apache.felix.scr.impl.manager.AbstractPrototypeRefPair.doUngetService(ScrComponentContext, T)

That is until we can get a better view of the correct specification behavior with respect to prototype service references.

@cziegeler
Copy link
Contributor

@tjwatson I am not 100% sure but I think we discussed how to handle multiple references to a single prototype from one component in the spec group and decided to handle this as a single reference which is then injected into the various fields/methods.

@tjwatson
Copy link
Member

@tjwatson I am not 100% sure but I think we discussed how to handle multiple references to a single prototype from one component in the spec group and decided to handle this as a single reference which is then injected into the various fields/methods.

I see that in https://docs.osgi.org/specification/osgi.cmpn/8.1.0/service.component.html#service.component-reference.scope which says:

prototype_required - For all references to a given bound service, each activated component instance must use a single, distinct service object. That is, for a given bound service, each component instance will use a distinct service object but within a component instance all references to the bound service will use the same service object.

But the problem is then later the spec says this which has some contradiction:

For a bound service of a reference with prototype or prototype_required reference scope, SCR must use a Service Objects object obtained from the OSGi Framework's service registry using the component's Bundle Context to get any service objects. If service objects for a bound service have been obtained and the service becomes unbound, SCR must unget any unreleased service objects using the Service Objects object obtained from the OSGi Framework's service registry using the component's Bundle Context. This means that if a component instance used a Component Service Objects object to obtain service objects, SCR must track those service objects so that when the service becomes unbound, SCR can unget any unreleased service objects.

The problem is implementing this unget of the service objects when the service is "unbound". What is suppose to happen if the same service is injected into more than one reference for a component but later only one is "unbound" from a dynamic optional reference? What the implementation does is unget all instances of the service object which is the same instance still bound to the other references. This is what leads to the problem. Once the other references un "unbound" SCR still has a reference to the injected object and it ungets it again. If we had instead let each reference get its own managed instance this would not be a problem. But as it stands now we have this problem where we are ungetting service objects when we shouldn't while there are still other references in the component that still need the service object.

@cziegeler
Copy link
Contributor

I am not sure if the second stated paragraph is a clear contradiction - but I get the problem with the current implementation of unget. Without any deeper thinking, I guess we need a usage (# references) counter. That would avoid the exception and would solve the edge case mentioned.

@juergen-albert
Copy link
Contributor Author

@tjwatson What you describe seems to be an issue as well, but I'm not sure if this is what is causing this issue here. In my case here, I have only on Reference here, but for some reason, SCR thinks it has the service injected twice, because the service provides 2 interfaces that both are instances of the requested interface A. Thus I'm not sure, if this is the same issue we have here.

I'm not sure if my solution is the right one and feels more like a workaround to the actual problem, that SCR sees one reference as 2 RefPais when I look at it today.

@cziegeler
Copy link
Contributor

@juergen-albert Can you share your component xml? or the code?

@juergen-albert
Copy link
Contributor Author

juergen-albert commented Jun 25, 2025

The test attached to this PR provokes the Behavior.

https://github.com/apache/felix-dev/pull/334/files

@tjwatson
Copy link
Member

What you describe seems to be an issue as well, but I'm not sure if this is what is causing this issue here.

It is related though because of this line of code in invokeUnbindMethod:

componentContext.getComponentServiceObjectsHelper().closeServiceObjects(refPair.getRef());

That ungets all known service objects that were obtained from the ServiceObjects from the framework. It then does the unget again for the actual reference from the field reference. But the real problem IMO is that it ungets all the known objects when the reference service is "unbound".

@tjwatson
Copy link
Member

That ungets all known service objects that were obtained from the ServiceObjects from the framework. It then does the unget again for the actual reference from the field reference. But the real problem IMO is that it ungets all the known objects when the reference service is "unbound".

What I think needs to happen here is that it should not call componentContext.getComponentServiceObjectsHelper().closeServiceObjects(refPair.getRef()); until all references for that same ServiceReference have been completely unbound.

But this still has weird corner cases if the ComponentServiceObjects was injected as a dynamic/optional reference and only that reference gets unbound (because of some target filter). It would seem like that situation should unget all the services obtained by that ComponentServiceObjects. But currently the implementation shared the same ComponentServicesObject instance for every reference in a component instance.

All of this would have been so much more clear if each reference used a different ComponentServicesObject to manage their individual instance. But the spec took a different approach which makes this more confusing and hard to implement.

@juergen-albert
Copy link
Contributor Author

How the hell did I manage to debug this setup the last time I worked on that?

@tjwatson The main issue, is that

refPairs = m_customizer.getRefs(trackingCount);

produces returns a collection with 2 RefPair which both represent the same Object. I know this is kind of what you referred to in your issue description above, in my case , I have only one Reference declared, so I would expect only one RefPair to be here.

@cziegeler
Copy link
Contributor

cziegeler commented Jun 25, 2025

that sounds like the wrong customizer is used, the static singleton one only returns 0 or 1

@tjwatson
Copy link
Member

produces returns a collection with 2 RefPair which both represent the same Object. I know this is kind of what you referred to in your issue description above, in my case , I have only one Reference declared, so I would expect only one RefPair to be here.

For what it is worth, I don't see that while debugging your test, but I also don't see the actual exception get thrown, instead it is only logged.

@juergen-albert
Copy link
Contributor Author

Strange. I can't confirm or deny what you see, as I'm unable to debug at the Moment. That the Exception is only logged is normal.

@tjwatson
Copy link
Member

How the hell did I manage to debug this setup the last time I worked on that?

Don't forget you have to uncomment this line in your test class to debug:

        // uncomment to enable debugging of this test class
        //        paxRunnerVmOption = DEBUG_VM_OPTION;

@juergen-albert
Copy link
Contributor Author

ahh, that makes sense :-) Thanks heaps. I'll take another look today and will post my findings.

@juergen-albert
Copy link
Contributor Author

I finally have the cause of the issue here in this case and I actually think my suggest fix is correct:

  1. DependencyManager.close() is called for org.apache.felix.scr.integration.components.felix6726.Consumer
  2. RefPair for with the Prototype Instance of org.apache.felix.scr.integration.components.felix6726.BImpl is found
  3. invokeUnbindMethod is called in
    invokeUnbindMethod(componentContext, boundRef, trackingCount.get(), edgeInfo);
  4. invokeUnbindMethod already closes and ungets the serivce in
    componentContext.getComponentServiceObjectsHelper().closeServiceObjects(refPair.getRef());
  5. then boundRef.ungetServiceObject is called afterwards and now the Exception is thrown.

@juergen-albert
Copy link
Contributor Author

Which after rereading @tjwatson initial comment shows that he was right as well, as he was already pointing at this issue.

@tjwatson
Copy link
Member

tjwatson commented Jul 8, 2025

I finally have the cause of the issue here in this case and I actually think my suggest fix is correct:

I have my doubts for cases that are not prototype references because in that case this will do nothing:

componentContext.getComponentServiceObjectsHelper().closeServiceObjects(refPair.getRef());

So I expect other reference types will be left with references that were never ungotten.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants