Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[osgi] Solutions for https://github.com/eclipse/microprofile/issues/33 #381

Merged
merged 2 commits into from
Jun 2, 2020
Merged

[osgi] Solutions for https://github.com/eclipse/microprofile/issues/33 #381

merged 2 commits into from
Jun 2, 2020

Conversation

rotty3000
Copy link
Contributor

No description provided.

@rotty3000 rotty3000 force-pushed the apply.slm branch 3 times, most recently from 1301e9b to bdcf2d0 Compare August 30, 2018 16:14
@rotty3000 rotty3000 mentioned this pull request Sep 6, 2018
Closed
@pavolloffay pavolloffay mentioned this pull request Sep 6, 2018
Closed
Emily-Jiang
Emily-Jiang requested changes Sep 6, 2018
View reviewed changes
api/bnd.bnd Outdated Show resolved Hide resolved
@rotty3000
Copy link
Contributor Author

rotty3000 commented Sep 6, 2018 via email

@rotty3000
Copy link
Contributor Author

rotty3000 commented Sep 6, 2018 via email

@Emily-Jiang
Copy link
Member

I disagree to update this in order to fix the issue highlighted. The issue is only a minor setter. This solution pushes OSGi environment to adopt Service Loader spec, which is a bigger change.

@rotty3000
Copy link
Contributor Author

rotty3000 commented Sep 7, 2018 via email

@rotty3000
Copy link
Contributor Author

rotty3000 commented Sep 7, 2018 via email

Emily-Jiang
Emily-Jiang reviewed Sep 11, 2018
View reviewed changes
api/bnd.bnd Outdated Show resolved Hide resolved
@rotty3000
Copy link
Contributor Author

rotty3000 commented Sep 11, 2018 via email

@Emily-Jiang
Copy link
Member

@rotty3000 I was aware of that Java Contract spec. Since no spec implements it, I think this is a bit awkward to force this requirement from MicroProfile to specify this requirement. I think, as you hinted, it is better to push from Jakarta EE direction to specify the capabilities and then MicroProfile can then require the capabilities.
In the meanwhile, for OSGi apps, I think you can do manifest rewrite to repackage the bundle to specify the requirement if you have fragment attached to specify the capability.

@rotty3000
Copy link
Contributor Author

rotty3000 commented Sep 17, 2018 via email •
edited
Loading

@Emily-Jiang
Copy link
Member

@rotty3000 I think it is better to drive from the provider rather than from consumer. The suitable fix is provide the capability from the Java APIs. I think it is better to fix in Jakarta EE. Once there is such capability, the consumer can specify the requirement. I don't think it can be easily work the other way around.

@eclipse-microprofile-bot
Copy link
Contributor

Can one of the admins verify this patch?

@dmlloyd
Copy link
Contributor

dmlloyd commented Mar 13, 2020

This discussion tapered out but the topic still seems relevant. Do we have enough OSGi experts active to continue the discussion?

@bjhargrave
Copy link
Member

Do we have enough OSGi experts active to continue the discussion?

Yes! I am willing to get more involved. We have new features in Bnd lately which can make this even easier to maintain. Any change would not introduce a direct dependency on any OSGI code at runtime.

@dmlloyd
Copy link
Contributor

dmlloyd commented Mar 17, 2020

Great! Maybe we can move this forward towards a resolution? What we want to be able to do, I think, is to be able to safely ensure that every OSGi platform is able to use ServiceLoader to locate the configuration provider resolver (and potentially other service provider implementations) in the same way, while also not relying on TCCL in a static context (which is broadly unpredictable). This eliminates the need to have an implementation setter (#468) and consequently gets rid of some tricky race conditions as a result.

WDYT?

@rotty3000
Copy link
Contributor Author

@bjhargrave, I just forced pushed a new change for this. Could you take a look at this approach?

@rotty3000
Copy link
Contributor Author

these build failures seem infra related.

@rotty3000
Copy link
Contributor Author

FYI, the exact branch passed on my own fork https://github.com/rotty3000/microprofile-config/actions/runs/65412890

Emily-Jiang
Emily-Jiang requested changes Mar 29, 2020
View reviewed changes
api/src/main/java/org/eclipse/microprofile/config/inject/ConfigProperty.java
@@ -100,6 +104,8 @@
@Qualifier
@Retention(RUNTIME)
@Target({METHOD, FIELD, PARAMETER, TYPE})
@Requirement(namespace = CDI_EXTENSION_PROPERTY, name = "org.eclipse.microprofile.config", effective = "active")
@Requirement(namespace = CDI_EXTENSION_PROPERTY, name = "org.eclipse.microprofile.config", resolution = OPTIONAL)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is very fishy. Why do you have to add Requirement twice? Why cannot it cope with both effective and resolution?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rotty3000 What is the goal here in attempting to generate 2 requirements in bundles using the ConfigProperty annotation? I am guessing your goals is to have a non-optional requirements at active time so provisioning resolutions will include a CDI extender bundle and to have an optional requirement at runtime so the bundle will resolve even when there is no bundle providing the CDI extender capability. Sort of a bridge to the future while still working with the past?

If so, we need to have some comments here explaining why we are doing this. Since this confused @Emily-Jiang.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we will also need to add some words to the Config spec so that CDI extender bundles provide this capability.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bjhargrave you are exactly correct!

BTW, in times past when I've discussed this with @pkriens and @tjwatson we've dubbed this combination of two requirements, one optional, one effective active as a weak requirement.

A weak requirement is used because IBM clearly does not want to strictly require Service Loader Mediator. This avoids that. Meanwhile, anyone who does want to use the resolver for assembly can simply enable active effectiveness.

About the OSGi CDI extension name, if folks are in agreement we could document that in the spec.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A weak requirement is used because IBM clearly does not want to strictly require Service Loader Mediator.

The case here is not about SLM. It is about the fact that there are some CDI extension implementations which do not provide the capability for resolve time.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, it's the exact same reason. If someone doesn't want to use OSGi CDI and manually wire things up it will still work.

Copy link
Member

@bjhargrave bjhargrave Mar 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this PR should put comments in the source code where we apply the weak requirements to document why there are 2 seemingly duplicate requirements and why we use effective:=active for non-runtime resolution scenarios.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds reasonable! I'll do that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was my original objection for this PR with the requirement for runtime to provide CDI capability. Otherwise the bundle does not resolve. I think with this weak requirement, it is reasonable change. Agreed with @bjhargrave to add comments to explain this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We also need to agree on, and document in the spec, the name of the CDI extension for Config. @rotty3000 proposes org.eclipse.microprofile.config which seems fine to me. But others (spec readers and implementors) will need to know the agreed upon name.

@Emily-Jiang
Copy link
Member

Great! Maybe we can move this forward towards a resolution? What we want to be able to do, I think, is to be able to safely ensure that every OSGi platform is able to use ServiceLoader to locate the configuration provider resolver (and potentially other service provider implementations) in the same way, while also not relying on TCCL in a static context (which is broadly unpredictable). This eliminates the need to have an implementation setter (#468) and consequently gets rid of some tricky race conditions as a result.

WDYT?

Well, I have not heard of anyone reporting any issues about the setter. I disagree to remove the setter for allowing the alternative ServiceLoader instead of using TCCL. This forces OSGi runtime to use ServiceLoader impl such as spifly etc, which is not a trival task to do.

@dmlloyd
Copy link
Contributor

dmlloyd commented Mar 30, 2020

Great! Maybe we can move this forward towards a resolution? What we want to be able to do, I think, is to be able to safely ensure that every OSGi platform is able to use ServiceLoader to locate the configuration provider resolver (and potentially other service provider implementations) in the same way, while also not relying on TCCL in a static context (which is broadly unpredictable). This eliminates the need to have an implementation setter (#468) and consequently gets rid of some tricky race conditions as a result.
WDYT?

Well, I have not heard of anyone reporting any issues about the setter. I disagree to remove the setter for allowing the alternative ServiceLoader instead of using TCCL. This forces OSGi runtime to use ServiceLoader impl such as spifly etc, which is not a trival task to do.

I reported issues about the setter. We initially were using it but it's inherently error-prone (due to race conditions between getting the initial configuration and setting the provider). It's better to do some work and have a reliable API than to have an unreliable API in the name of expediency.

bjhargrave
bjhargrave suggested changes Mar 30, 2020
View reviewed changes
api/src/main/java/org/eclipse/microprofile/config/inject/ConfigProperty.java
@@ -100,6 +104,8 @@
@Qualifier
@Retention(RUNTIME)
@Target({METHOD, FIELD, PARAMETER, TYPE})
@Requirement(namespace = CDI_EXTENSION_PROPERTY, name = "org.eclipse.microprofile.config", effective = "active")
@Requirement(namespace = CDI_EXTENSION_PROPERTY, name = "org.eclipse.microprofile.config", resolution = OPTIONAL)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rotty3000 What is the goal here in attempting to generate 2 requirements in bundles using the ConfigProperty annotation? I am guessing your goals is to have a non-optional requirements at active time so provisioning resolutions will include a CDI extender bundle and to have an optional requirement at runtime so the bundle will resolve even when there is no bundle providing the CDI extender capability. Sort of a bridge to the future while still working with the past?

If so, we need to have some comments here explaining why we are doing this. Since this confused @Emily-Jiang.

api/src/main/java/org/eclipse/microprofile/config/inject/ConfigProperty.java
@@ -100,6 +104,8 @@
@Qualifier
@Retention(RUNTIME)
@Target({METHOD, FIELD, PARAMETER, TYPE})
@Requirement(namespace = CDI_EXTENSION_PROPERTY, name = "org.eclipse.microprofile.config", effective = "active")
@Requirement(namespace = CDI_EXTENSION_PROPERTY, name = "org.eclipse.microprofile.config", resolution = OPTIONAL)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we will also need to add some words to the Config spec so that CDI extender bundles provide this capability.

api/src/main/java/org/eclipse/microprofile/config/spi/ConfigProviderResolver.java
@@ -36,6 +38,7 @@
* @author <a href="mailto:[email protected]">Romain Manni-Bucau</a>
* @author <a href="mailto:[email protected]">Emily Jiang</a>
*/
@ServiceConsumer(value = ConfigProviderResolver.class, effective = "active")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume this is also active for similar reasons as mentioned above. We should also document why we do active here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly! Since there has been resistance to the SLM solution, this makes the feature usable during assembly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whether this gets documented in the spec is up for debate. I think we could come up with standard language would simplify applying this to other MP specs if desired. It would largely be the same words for every spec except for the identity (which I'd suggest simply going with the bsn of the API for consistency and avoid confusion).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this work, given that, at the moment, the service loading isn't done using the thread context classloader?

See line 110:

                instance = loadSpi(ConfigProviderResolver.class.getClassLoader());

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this work, given that, at the moment, the service loading isn't done using the thread context classloader?

Yes. For example, you can use a fragment to attach the META-INF/service resource and add any necessary Import-Package/DynamicImport-Package statement to this bundle.

Apache Aries SPI Fly is another option.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeap! The SML spec never limited exactly how the loading was done and so within the past two years the Apache Aries SPI Fly implementation was enhanced to support the two argument load(Class, ClassLoader) method. The heuristic is to use the passed ClassLoader as fallback if no providers are found. So no code change here is required.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SML

SLM :-)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, a thread context class loader does not help here as trying to use it would introduce timing issues similar to using setInstance. You would have to arrange to set the TCCL properly before being the first one to cause the ServiceLoader to load the impl class.

To be fair, there are also timing issues with SPI Fly in that you have to ensure it is started (perhaps via start levels) before anyone causes the ServiceLoader call to be made.

Using a fragment is probably the safest as it will be attached at resolve time which is by definition before any code in the host bundle can execute.

api/pom.xml Outdated Show resolved Hide resolved
api/bnd.bnd Outdated
javax.enterprise.util',\
osgi.contract;\
osgi.contract=JavaInject;\
version:Version='1.0';\
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This contract is not in the generated manifest? (when built on my local system).

Does is matter that version is not List<Version>?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, this is was a weird issue. I tried in both forms List and scalar, but neither caused the contract to be added. Thoughts? I suspect the issue is because the only reference is as a meta-annotation. Thoughts?

@rotty3000
Copy link
Contributor Author

Please see updates. I've added some source comments and made a few other changes recommended by @bjhargrave.

@rotty3000
Copy link
Contributor Author

@Emily-Jiang @bjhargrave just let me know if you have any suggestions to clearify the comments.

@rotty3000
Copy link
Contributor Author

Also, if you wish to add some wording to the spec please let me know where/in which section you'd like me to stick those and I can try a first draft.

@rotty3000 rotty3000 force-pushed the apply.slm branch 3 times, most recently from d91e521 to 75d0a73 Compare March 30, 2020 18:13
@rotty3000
Copy link
Contributor Author

@bjhargrave I think this addresses the issues we discussed.

bjhargrave
bjhargrave suggested changes Mar 30, 2020
View reviewed changes
api/bnd.bnd Outdated Show resolved Hide resolved
@rotty3000
apply service loader mediator (via bnd annotations), osgi.cdi (via os…
2fbf273 
…gi bundle annotations) & osgi contracts in a passive way

Signed-off-by: Raymond Augé <[email protected]>
@rotty3000
eliminate transfer progress logging to simplify reviewing CI build logs
9941557 
Signed-off-by: Raymond Augé <[email protected]>
@Emily-Jiang Emily-Jiang mentioned this pull request Apr 2, 2020
Closed
@Emily-Jiang
Copy link
Member

Emily-Jiang commented Apr 2, 2020 •
edited
Loading

We are trying out on Open Liberty to see whether the bundle can be resolved.

@rotty3000
Copy link
Contributor Author

rotty3000 commented Apr 3, 2020 via email

@rotty3000
Copy link
Contributor Author

rotty3000 commented Apr 3, 2020 via email •
edited
Loading

@bjhargrave
Copy link
Member

Also there is still timing issue with fragments, since the host could already be resolved. You'd have to manually reresolve the host.

Well this is only possible if one installs the fragment into an already running framework. For any typical MP Config supporting runtime, the mp config api host bundle and this fragment would already have been installed prior to running any application. So there would be no timing issue in any normal scenario.

@rotty3000
Copy link
Contributor Author

Also there is still timing issue with fragments, since the host could already be resolved. You'd have to manually reresolve the host.

Well this is only possible if one installs the fragment into an already running framework. For any typical MP Config supporting runtime, the mp config api host bundle and this fragment would already have been installed prior to running any application. So there would be no timing issue in any normal scenario.

This is also true for framework extensions which SPI Fly provides.

@rmannibucau
Copy link

Hi all, what's the status on that? It will be almost 2 years old and since OSGi-CDI is out and starts to be very stable, OSGi ecosystem is a real target of Microprofile now.
I'd like to ensure it is handled at spec level or we'll have to fork the spec at apache to enable these users.

@Emily-Jiang
Copy link
Member

We tested this on Open Liberty and used fragment solution recommended by @bjhargrave and the bundles seem to resolve ok.

@Emily-Jiang Emily-Jiang merged commit b7415ed into eclipse:master Jun 2, 2020
@geoand geoand mentioned this pull request Jun 2, 2021
Closed
@radcortez radcortez mentioned this pull request Sep 17, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Azquelt Azquelt Azquelt left review comments

@bjhargrave bjhargrave bjhargrave approved these changes

@Emily-Jiang Emily-Jiang Emily-Jiang approved these changes

@struberg struberg Awaiting requested review from struberg

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@rotty3000 @Emily-Jiang @eclipse-microprofile-bot @dmlloyd @bjhargrave @rmannibucau @struberg @Azquelt