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

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

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7b737e4
Draft for [connect] allow registration of FrameworkUtilHelper directly
laeubi Jul 27, 2022
f2f34f6
Use a more less intrusive approach...
laeubi Jul 27, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions bundles/org.eclipse.osgi/META-INF/services/org.osgi.framework.connect.FrameworkUtilHelper
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
org.eclipse.osgi.internal.framework.ConnectFrameworkUtilHelper
26 changes: 26 additions & 0 deletions ...se.osgi/container/src/org/eclipse/osgi/internal/framework/ConnectFrameworkUtilHelper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
package org.eclipse.osgi.internal.framework;

import java.util.Objects;
import java.util.Optional;
import java.util.Set;
import java.util.concurrent.ConcurrentHashMap;
import org.osgi.framework.Bundle;
import org.osgi.framework.connect.FrameworkUtilHelper;

public class ConnectFrameworkUtilHelper implements FrameworkUtilHelper {
static final Set<FrameworkUtilHelper> connectHelpers = ConcurrentHashMap.newKeySet();

@Override
public Optional<Bundle> getBundle(Class<?> classFromBundle) {
return connectHelpers.stream().filter(Objects::nonNull)
.flatMap(helper -> helper.getBundle(classFromBundle).stream()).findFirst();
}

public static void add(FrameworkUtilHelper moduleConnector) {
connectHelpers.add(moduleConnector);
}

public static void remove(FrameworkUtilHelper moduleConnector) {
connectHelpers.remove(moduleConnector);
}
}
22 changes: 22 additions & 0 deletions .../org.eclipse.osgi/container/src/org/eclipse/osgi/internal/framework/EquinoxContainer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.Set;
import java.util.WeakHashMap;
import java.util.concurrent.ConcurrentHashMap;
Expand Down Expand Up @@ -57,6 +58,7 @@
import org.osgi.framework.FrameworkUtil;
import org.osgi.framework.connect.ConnectContent;
import org.osgi.framework.connect.ConnectModule;
import org.osgi.framework.connect.FrameworkUtilHelper;
import org.osgi.framework.connect.ModuleConnector;
import org.osgi.util.tracker.ServiceTracker;

Expand Down Expand Up @@ -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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

ConnectFrameworkUtilHelper.add((FrameworkUtilHelper) moduleConnector);
}
moduleConnector.initialize(fwkStore, Collections.unmodifiableMap(config));
}

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

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

@tjwatson tjwatson Jul 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

FrameworkUtilHelper helper = (FrameworkUtilHelper) connectModules.moduleConnector;
Optional<Bundle> bundle = helper.getBundle(clazz);
if (bundle.isPresent()) {
return bundle.get();
}
}
Bundle b = FrameworkUtil.getBundle(clazz);
if (b != null) {
return b;
Expand Down Expand Up @@ -214,6 +226,10 @@ public boolean isProcessClassRecursionSupportedByAll() {
}

void init() {
if (connectModules.moduleConnector instanceof FrameworkUtilHelper) {
FrameworkUtilHelper helper = (FrameworkUtilHelper) connectModules.moduleConnector;
ConnectFrameworkUtilHelper.add(helper);
}
eventPublisher.init();
synchronized (this.monitor) {
serviceRegistry = new ServiceRegistry(this);
Expand All @@ -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(() -> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this done async here?

Copy link
Member Author

Choose a reason for hiding this comment

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

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...?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

ConnectFrameworkUtilHelper.remove(helper);
});
}
currentExecutor.shutdown();
}

Expand Down