Draft for [connect] allow registration of FrameworkUtilHelper directly #90

laeubi · 2022-07-27T16:23:07Z

A draft for the idea of

[proposal][connect] allow registration of FrameworkUtilHelper directly osgi/osgi#463

so it becomes more "visual" ...

tjwatson · 2022-07-27T17:39:21Z

bundles/org.eclipse.osgi/osgi/src/org/osgi/framework/connect/ConnectFramework.java

@@ -0,0 +1,25 @@
+package org.osgi.framework.connect;


We cannot prototype changes to the org.osgi packages. We have to keep these packages signatures the same as what the OSGi specification defines. Otherwise we will fail the OSGi TCK signature tests.

Not sure if your intention was to merge something ahead of getting a OSGi specification update first.

What do you suggest? Can we add a new interface or will this also be complained? this might be placed in an equinox specific package then and one need to use instanceof so we can keep the original signatures here?

tjwatson · 2022-07-27T18:01:35Z

bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/launch/EquinoxFactory.java

 	}

 	@Override
-	public Framework newFramework(Map<String, String> configuration, ModuleConnector moduleConnector) {
-		return new Equinox(configuration, moduleConnector);
+	public ConnectFramework newFramework(Map<String, String> configuration, ModuleConnector moduleConnector) {


A complication with this approach is the two views of the system bundle (Framework) that Equinox currently has. We have the external view from the launcher Equinox class that implements Framework as well as the internal view with the org.eclipse.osgi.internal.framework.EquinoxBundle.SystemBundle class. The Equinox class ends up delegating to the internal representation of the system bundle class org.eclipse.osgi.internal.framework.EquinoxBundle.SystemBundle after it has created the EquinoxContainer.

This fact allowed you to provide an alternative view (ConnectFramework) externally so that external launchers can add their helpers. This is good because internally bundles can cast Bundle with ID=0 to Framework but they would not be able to cast to ConnectFramework and therefore never be able to add helpers internally (something I wanted to block). But other OSGi framework implementations may not implement the system bundle this way. They may actually have the internal system bundle representation be the exact same object returned from the FrameworkFactory.

An alternative approach could be to have an interface which ModuleConnector implementations can also implement that has a getFrameworkUtilHelper() method. When a framework instance is initialized with a ModuleConnector that provides a helper then the framework can get that helper registered in some implementation specific way.

This also allows us to avoid having to manage the helpers with add/remove methods which can lead to leaky bugs. Instead when a framework is stopped it will need to clean up the ModuleConnector helper from its internal store so it is no longer called by the FrameworkUtil class. If that stopped framework instance is initialized again then the helper from the ModuleConnector would be applied again to the frameworks helper automatically.

I propose you do not prototype any changes to the org.osgi package. Instead you could add an interface to the org.eclipse.osgi.launch package that holds the Optional<FrameworkUtilHelper> getFrameworkUtilHelper() method which we would like to eventually get added (as a default method returning empty) to the ModuleConnector interface in a future OSGi specification.

Before we merge anything like that I would like to get some agreement on this direction in the OSGi Spec working group to avoid adding API to Equinox that may be drastically different from what would eventually be spec'ed in OSGi.

An alternative approach could be to have an interface which ModuleConnector implementations can also implement that has a getFrameworkUtilHelper() method. When a framework instance is initialized with a ModuleConnector that provides a helper then the framework can get that helper registered in some implementation specific way.

Thinking more about this, maybe one can simply say, if ModuleConnector also implements FrameworkUtilHelper that would be used? I think this would be most "unintrusive" and even only need a spec adjustment instead of changing any code or interfaces, I'll update the PR accordingly so we can see how it behaves...

Before we merge anything like that I would like to get some agreement on this direction in the OSGi Spec working group to avoid adding API to Equinox that may be drastically different from what would eventually be spec'ed in OSGi.

If I remember correctly the next two calls are canceled, maybe we can still use the time to came up with something we are comfortable with so its much easier to discuss in the next spec call then, if we can agree on a method we might even can prepare a PR to change the spec accordingly as a base for the discussion, I just never did this before so no idea how complicate it would be to do so...

Thinking more about this, maybe one can simply say, if ModuleConnector also implements FrameworkUtilHelper that would be used? I think this would be most "unintrusive" and even only need a spec adjustment instead of changing any code or interfaces, I'll update the PR accordingly so we can see how it behaves...

Sure, that would be the least amount of API change and something we could prototype without breaking any signature tests. But it is hard to adequately document such implicit contracts without a visible API signature change. I think I prefer the more explicit API change that documents the behavior clearly.

I just never did this before so no idea how complicate it would be to do so...

Specs move slow, so I would be prepared to have an interim solution that is Equinox specific for a while. I just want to ensure we are happy to support that Equinox specific API for the long run.

But it is hard to adequately document such implicit contracts without a visible API signature change. I think I prefer the more explicit API change that documents the behavior clearly.

What do you think about using the (implicit) solution for this PR (to not break signatures) and suggest to the Spec that ModuleConnector simply extends FrameworkUtilHelper to make it explicit? That sounds like the most simple solution as it also removes any need for specifying that a (whatever) getter always return the same instance and so on.

What do you think about using the (implicit) solution for this PR (to not break signatures) and suggest to the Spec that ModuleConnector simply extends FrameworkUtilHelper to make it explicit? That sounds like the most simple solution as it also removes any need for specifying that a (whatever) getter always return the same instance and so on.

Seems like a reasonable starting point. It also reduces the risk to the Equinox API. Unless/until we document this somehow it remains an internal dependency. Tycho can certainly depend on the internal, but we would be free to remove the internal support until we documented it somehow as a proper external. I worry about such things because no API tooling can detect changes to such implicit support. Hence I would prefer it remain something internal that we can play around with until there is a spec'ed way to do it.

At the spec level we would have to determine if extending FrameworkUtilHelper can happen in a non-major version (breaking) way by adding the required default implementation directly to ModuleConnector. ModuleConnector is a consumer type so we need to add methods to it carefully.

But thinking about that, it would then force the framework implementation to always delegate to the supplied ModuleConnector even though it never provides an overriding org.osgi.framework.connect.FrameworkUtilHelper.getBundle(Class<?>) method. That may be a contentious point to discuss at the spec level because it is not something a ModuleConnector can opt into with a mix-in interface like we would do at first in Equinox with optionally allowing a ModuleConnector to implement FrameworkUtilHelper. Ultimately that may result in OSGi deciding to just support the mix-in approach. But then they would at least add that to the javadoc of ConnectFrameworkFactory and ModuleConnector

Hence I would prefer it remain something internal that we can play around with until there is a spec'ed way to do it.

That would be fine for me, I already got the official way working for 80% of the cases and switching to felix or some other osgi impl seems not really an option here.

At the spec level we would have to determine if extending FrameworkUtilHelper can happen in a non-major version (breaking) way by adding the required default implementation directly to ModuleConnector. ModuleConnector is a consumer type so we need to add methods to it carefully.

I more though about ModuleConnector extends FrameworkUtilHelper so FrameworkUtilHelper remains unchanged or do I understand wrong?

But thinking about that, it would then force the framework implementation to always delegate to the supplied ModuleConnector even though it never provides an overriding

I think this is acceptable and assume most (if not all) implementations will migrate to the ModuleConnector approach as the SPI one is really hard to get right... But if all this is a concern, I think we can also add a method to ConnectFrameworkFactory that accepts a ModuleConnector and a FrameworkUtilHelper that should then only be a provider type change ...

bundles/org.eclipse.osgi/META-INF/services/org.osgi.framework.connect.ConnectFrameworkFactory

tjwatson · 2022-07-28T18:11:19Z

To move this forward you need to add tests to org.eclipse.osgi.tests.bundles.ConnectTests to have a ModuleConnector that implements FrameworkUtilHelper to test this. Also, you should move the adding of the helper from the ModuleConnector to the method org.eclipse.osgi.internal.framework.EquinoxContainer.init() and then you need to remove the helper from org.eclipse.osgi.internal.framework.EquinoxContainer.close() method.

tjwatson · 2022-07-28T18:15:48Z

...les/org.eclipse.osgi/container/src/org/eclipse/osgi/internal/framework/EquinoxContainer.java

@@ -174,6 +179,13 @@ public EquinoxLogServices getLogServices() {
 	}

 	public Bundle getBundle(Class<?> clazz) {
+		if (connectModules.moduleConnector instanceof FrameworkUtilHelper) {


We should not need to do this if the helper is added to the ConnectFrameworkUtilHelper and plugged into FrameworkUtil. Will this not lead to it being called twice?

I think a Connect user will either register it as an SPI or through the creation directly.
The other idea was that we always give the chance first the one provided by the connect factory because I already found a little problem when running multiple frameworks that one has to find out what is the right one for that framework.

My point is that I don't know why you call the ModuleConnector as a helper directly here when it should have been automatically added to the ConnectFrameworkUtilHelper and called below when we invoke FrameworkUtil.getBundle.

As for giving this helper a chance first. I don't think we should do that. Any helper has to be able to figure this stuff about the framework it is from itself because any code not using the deprecated PackageAdmin will instead directly call FrameworkUtil.getBundle. But calling the same helper twice doesn't seem correct.

tjwatson · 2022-07-28T18:15:54Z

...les/org.eclipse.osgi/container/src/org/eclipse/osgi/internal/framework/EquinoxContainer.java

@@ -154,6 +156,9 @@ private static void initConnectFramework(ModuleConnector moduleConnector, Equino
 		final File fwkStore = new File(configUrl.getPath());
 		@SuppressWarnings({"rawtypes", "unchecked"})
 		Map<String, String> config = (Map) equinoxConfig.getInitialConfig();
+		if (moduleConnector instanceof FrameworkUtilHelper) {


I don't think this is necessary if you are adding during init.

I was not really sure waht should be the very first time it is registered, e.g after creation of the factory? or after/before init? Or maybe when start is called?

I think we should start with it only being registered in init

tjwatson · 2022-07-28T18:16:36Z

...les/org.eclipse.osgi/container/src/org/eclipse/osgi/internal/framework/EquinoxContainer.java

@@ -240,6 +256,12 @@ void close() {
 		currentStorage.close();
 		// Must be done last since it will result in termination of the
 		// framework active thread.
+		if (connectModules.moduleConnector instanceof FrameworkUtilHelper) {
+			FrameworkUtilHelper helper = (FrameworkUtilHelper) connectModules.moduleConnector;
+			currentExecutor.execute(() -> {


Why is this done async here?

This is so this happens after all pending task of the executor as a very last action ... because we don't wait here to the shutdown of the executor.... but maybe that is not necessary...?

I would just do it synchronously. If we run into an issue with that then we can understand if something else is needed.

Draft for [connect] allow registration of FrameworkUtilHelper directly

7b737e4

See osgi/osgi#463

laeubi requested a review from tjwatson July 27, 2022 16:23

tjwatson reviewed Jul 27, 2022

View reviewed changes

bundles/org.eclipse.osgi/META-INF/services/org.osgi.framework.connect.ConnectFrameworkFactory Outdated Show resolved Hide resolved

laeubi force-pushed the draft-framework-connect branch from 03598c6 to ffb173c Compare July 28, 2022 04:39

Use a more less intrusive approach...

f2f34f6

laeubi force-pushed the draft-framework-connect branch from ffb173c to f2f34f6 Compare July 28, 2022 04:57

tjwatson reviewed Jul 28, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Draft for [connect] allow registration of FrameworkUtilHelper directly #90

Draft for [connect] allow registration of FrameworkUtilHelper directly #90

laeubi commented Jul 27, 2022

tjwatson Jul 27, 2022

laeubi Jul 27, 2022

tjwatson Jul 27, 2022

laeubi Jul 27, 2022

tjwatson Jul 27, 2022

tjwatson Jul 27, 2022

laeubi Jul 28, 2022

tjwatson Jul 28, 2022

laeubi Jul 28, 2022

tjwatson commented Jul 28, 2022

tjwatson Jul 28, 2022 •

edited

Loading

laeubi Jul 28, 2022

tjwatson Jul 28, 2022

tjwatson Jul 28, 2022

laeubi Jul 28, 2022

tjwatson Jul 28, 2022

tjwatson Jul 28, 2022

laeubi Jul 28, 2022

tjwatson Jul 28, 2022

Draft for [connect] allow registration of FrameworkUtilHelper directly #90

Are you sure you want to change the base?

Draft for [connect] allow registration of FrameworkUtilHelper directly #90

Conversation

laeubi commented Jul 27, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tjwatson commented Jul 28, 2022

tjwatson Jul 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tjwatson Jul 28, 2022 •

edited

Loading