-
Notifications
You must be signed in to change notification settings - Fork 26
OSGi Service as Global EPackageRegistry #55
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
OSGi Service as Global EPackageRegistry #55
Conversation
95cbb74 to
a5c7b4f
Compare
|
All the tests run as OSGi applications. But I guess you want to launch in some special way and it looks like you have some more special fancy test infrastructure/libraries that you use... |
|
I've not looked in great detail at the following PR which seems somewhat related at a glance: |
Related but something different. If I understand Hannes PR correctly, it collects .ecore files that are marked in the Manifest of a Bundle and registers the packages, similar to what the generated code does. I however want to have a possiblity to replace the static package registry completly, without the use of the system property or the Extensionpoint. |
Am I blind? Where can I find them? My tests are not really that special and should be adaptable to your tests with minimal effort. |
|
There's this: https://github.com/eclipse-emf/org.eclipse.emf/tree/master/tests/org.eclipse.emf.test.core The build launches tests like this: The IDE should look like this: |
|
Thanks heaps. Before I put in more work, by migrating my Tests: What is your general feeling towords this PR? Would this be something you would accept, given the tests and details work out? |
a5c7b4f to
b521136
Compare
|
@merks would you mind killing this job: https://ci.eclipse.org/emf/job/build/job/PR-55/2/ I produced a deadlock situation and I'm not sure, what timeout the job has until the Fix can be build. (Sorry for that) |
|
I really need some time to review this before I can have a well-informed opinion and I am horribly swamped these days and I'm out of office next week... I will definitely get fussy at some point about a space between |
No Problem and thank you.
I have tried to match your code style as best, as I was able. I imported the project via the Oomph setup, but there have been no codestyles attached. Do you have anything I can use or was I doing anything wrong during installation? |
|
@merks did you find the time to take a look at the PR in general? |
merks
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before I go much farther, the "scary" catches during startup look scary to me. They appear to mask or work around unknown problems and to open the potential to simply swallow real problems that might really need attention in the future...
It's also not entirely clear why using the extension point to add your registry implementation isn't sufficient. Why must you change the default used by everyone with something different? I have real concerns about everyone ending up using a different implementation after 20 years of using the current one. I have client with models from hell with 500 interdependent packages. This low level stuff kicks in during class initialization such that behavior changes have the potential to introduce new problems I've not seen before. Especially getEPackage can lead to reentrant behavior that might become problematic...
| try { | ||
| ExtensionProcessor.internalProcessExtensions(); | ||
| } catch (NoClassDefFoundError e) { | ||
| // If EMF runs in a non Equinox OSGi Framework, it occasionally runs in some NoClassDefFoundError Exceptions, that can safely be ignored. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there some way to detect when there is a non-Equinox OSGi framework running? The "occasionally" comment does not give me a warm fuzzy feeling about how much has been initialized or not. Furthermore, there's a bunch of stuff in EMF that was designed so that EMF runs on Felix but now there are some problems that I don't know about?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First of: EMF works perfectly fine in Felix! So there is in general nothing to worry about on your end here.
However :-), we are using QVT quite a lot and we are in some cases bound to Felix. QVT however has never gotten to the point where you are, so it can run without the usual org.eclipse.core.runtime dependency. Even with the supplement is does not work. Thus, we have to jump through a lot of hoops and provide a really scetchy Bundle, that sells QVT the notion, that it has everything it needs. This unfortunately has the consequence, that the EMFPlugin.IS_ECLIPSE_RUNNING suddenly also thinks it is true. This leads in the end to it expecting the ExtensionRegistry to be around, which we don't have and never will have.
We are currently working on QVT, to become independent of Equinox, but we don't know when and how this will be finished. Thus this is more of a precaution, that the Activator runs though, even if we run in scetchy mode.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is still here and still makes me really uncomfortable. How much has been properly initialized? What problems are we masking in the future.
| { | ||
| return new Implementation(); | ||
| } catch (Throwable t) { | ||
| return new PlainOSGiBundleActivator(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This too just looks scary. Something/anything goes horribly wrong and then the other thing kicks in as a substitute. In what case is such a thing needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I only used what you already provided. When EMF runs in Felix, it will always run into the catch block, as a NoClassDefFoundError will be thrown when the createBundle method is called, as in Felix you will not find the EclipsePlugin Implementation inherits from.
Thinking about it, it might be a better solution in general to use the EMFPlugin.IS_ECLIPSE_RUNNING as an indicator on what todo, instead of accepting the NoClassDefFoundError.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still catching a Throwable. So absolute anything could go horribly wrong and we'll just do something different, quietly not even processing plugin.xml. That's not okay. 😢
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what you mean? createBundleHelper catches the Throwable, but I'm not sure what I can do about it, as this is the mechanism that was in place before. We can at least log an error in the OSGiDelegatingBundleActivator in such a case.
| * In Case we are not running in Eclipse, this BundleActivator will be used | ||
| * @since 2.40 | ||
| */ | ||
| public static class PlainOSGiBundleActivator implements BundleActivator { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was going to ask why this needs to be in a separate class? But then I see later that this class is created if you can't create an Implementation instance. Again, I don't get warm fuzzy feelings about that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Implementation you provide, extends EclipsePlugin and will fail outside Eclipse. Thus I need a "Plain" BundleActivator, that will always work. Thus, this is a separate Class, which is used by the "Eclipse" Activator or standalone if we are not running in Eclipse.
|
Maybe some general Context. The way I implemented this, nothing should change for anybody. In normal JavaSE nothing changes. In any OSGi Environment, there will always be a default Registry, that can be overwritten via the System property or ExtensionPoint. Thus the existing mechanisms will always take precedence over the new mechanism. We however don't have any extension points and never will have them. Thus there is no way for us to provide our own registry. In my opinion the static registry should be used by generated code or ecore deployed with a Bundles. We however have a System, where besied the generated Code, EPackages can dynamically come and go and must be available without a ResourceSet around. Thus I need a way to exchange the Registry. I understand worries about the reentrent behavior. This is the reason, I introduced the lock and the only moment it blocks, is during the arrival of a new PackageRegistry Service. If one goes down the rabbit hole, to provide your own Registry, you are already in dangerous territory anyway and can screw up the whole system. |
|
I think it always replaces the registry with your new registry: What if we provided a way to specify via a system property both the bundle name and the class name, I'd prefer to introduce the least amount of change with the least risk of regression with the least amount of new code to maintain. |
|
This would not necessarily work, as we can't guarantee that the bundle, the property would name is already installed and resolved at the time this code is called. |
|
We could use a system property as a feature toggle and have the part deactivated by default. |
I'm really uncomfortable with not understanding exactly was is present in the runtime and what is not present, what's failing, why it's failing, and so on. Certainly clearly documented system property guards that have no impact by default would make me feel less like I'm risking regressions I don't even understand. Does the class org.eclipse.emf.ecore.plugin.OSGiDelegatEPackageRegistry need to be public API or could it be completely hidden, e.g., as a private static class in EcorePlugin? I would feel better to make as little new API commitment as possible... |
I will make the change. |
Not sure why I made it public. I can make it an internal inner class. |
b521136 to
cfed3c1
Compare
|
The Platform's RC1 is this week and for that I need to build EMF RC1, so I can't make any promises... |
|
Would be nice, if it could makle the release. There is currently one Test failing |
|
Yes, that's unrelated and something that I fixed just a while back and the master build does work: https://ci.eclipse.org/emf/job/build/job/master/478/ Anyway I must run now... I'll look at the PR tomorrow and run a local Tycho build... |
cfed3c1 to
c935cc0
Compare
merks
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In one place you will create a different activator when goodness-knows-what fails and in another place you won't complete processing the extensions when some class isn't found. This is just screaming for problems to happen and quietly ignored. Apparently there are two different problems that need masking/therapy. Certainly we can't push this through with this approach with RC1 looming. Can these be guarded with the system property instead?
| <stringAttribute key="org.eclipse.jdt.launching.PROJECT_ATTR" value="org.eclipse.emf.test.core"/> | ||
| <stringAttribute key="org.eclipse.jdt.launching.SOURCE_PATH_PROVIDER" value="org.eclipse.pde.ui.workbenchClasspathProvider"/> | ||
| <stringAttribute key="org.eclipse.jdt.launching.VM_ARGUMENTS" value="-Dorg.eclipse.emf.test.core.ecore.EcoreValidationTest.validateAllRegisteredModels=false -ea"/> | ||
| <stringAttribute key="org.eclipse.jdt.launching.VM_ARGUMENTS" value="-Dorg.eclipse.emf.test.core.ecore.EcoreValidationTest.validateAllRegisteredModels=false -Dorg.eclipse.emf.ecore.EPackage.Registry.OSGI_SERVICE=true -ea"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we test the new registry but no longer test the one everyone else uses.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have an issue with the testing. In order to properly make sure to test both Versions, I believe we would have to run the tests at least twice. One round with the Flag active and on with without. It would be even better to have a third test run, with e.g. a Felix instead of the Equinox with Eclipse. I hove no clue how to do this though with tycho. Do you?
| try { | ||
| ExtensionProcessor.internalProcessExtensions(); | ||
| } catch (NoClassDefFoundError e) { | ||
| // If EMF runs in a non Equinox OSGi Framework, it occasionally runs in some NoClassDefFoundError Exceptions, that can safely be ignored. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is still here and still makes me really uncomfortable. How much has been properly initialized? What problems are we masking in the future.
| { | ||
| return new Implementation(); | ||
| } catch (Throwable t) { | ||
| return new PlainOSGiBundleActivator(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still catching a Throwable. So absolute anything could go horribly wrong and we'll just do something different, quietly not even processing plugin.xml. That's not okay. 😢
The default EPackage Registry can be overwritten in mutlple ways (System property or ExtensionPoint). With this, if non of the other mechanisms apply, there now can be a service that acts as the static Registry. Signed-off-by: Juergen Albert <[email protected]>
c935cc0 to
ca1b1c3
Compare
|
Why the hell, don't I get any notifications when something happens to this PR... |
|
I do believe I found a better solution, to really figure out, if Eclipse is truly running. With this, I can avoid the catch constructs, that make make you rightfully nervous. |
|
Yes, something I get the sense that I miss notifications, e.g.,. when a PR is opened or a question is asked... FYI, I think the runway has grown to short. Next week I have to make a release build for the Platform RC2... |
|
Understood and no Problem. We can use the SNAPSHOT later on, when we are through. |
|
Running the tests multiple times is feasible/reasonable. Testing against Felix would be an interesting improvement. With the release cycle winding down, and with other demands on my time, I dropped the ball a bit here. Sorry about that. |
|
I think this is heading in the right direction now... I have a general question now about what's available and what is not. Specifically I think maybe the org.eclipse.core.runtime is present. But I think you are trying to exclude the org.eclipse.equinox.registry bundle. The historical support for running on Felix was designed to include that bundle. I ask because I think the class org.eclipse.core.runtime.Plugin is available because org.eclipse.core.runtime.Platform is available from the same bundle. As suchI think we don't need a separate PlainOSGiBundleActivator but rather need a guard that avoid calling and fold the implementation details into the base class. And then maybe we can use USE_OSGI_SERVICE_AS_EPACKAGE_REGISTRY as the guard for the above (it should be private and final) and don't need to change the meaning/implementation of IS_ECLIPSE_RUNNING. Or might one want the service-based registry when Eclipse is really running? Anyway, I think it's headed in a more comfortable direction... |
|
If we use QVT in a Project, we have our sketchy Thus, we need 2 Versions of the Activator if the mechanism should work in both scenarios. The alternative would be to get rid of the EclipsePlugin, which would be a bigger operation, even though also possible. |
merks
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a number of hole in the locking via the views that are returned. Perhaps all those views should be of an modifiable view of the underlying registry.
Maybe you could try to address some of these issues before try to take this further.
| * Which is used until the service comes around. All write operations will be | ||
| * stored with the backup as well, so it can take up its role, when the service | ||
| * goes away. When a suitable Service comes around, all statically added |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The write-through comment doesn't appear to be true. See comments below.
| if (delegate == null) { | ||
| backup.putAll(m); | ||
| } else { | ||
| delegate.putAll(m); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does not write through to the backup if there is a delegate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ohh, that seems like a classic copy and waste error. Yes it should all ways call putAll on backup.
| if (delegate == null) { | |
| backup.putAll(m); | |
| } else { | |
| delegate.putAll(m); | |
| } | |
| backup.putAll(m); | |
| if (delegate != null) { | |
| delegate.putAll(m); | |
| } |
| if (delegate == null) { | ||
| backup.clear(); | ||
| } else { | ||
| delegate.clear(); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does not write through to the delegate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| if (delegate == null) { | |
| backup.clear(); | |
| } else { | |
| delegate.clear(); | |
| } | |
| backup.clear(); | |
| if (delegate == null) { | |
| delegate.clear(); | |
| } |
| if (delegate == null) { | ||
| return backup.keySet(); | ||
| } else { | ||
| return delegate.keySet(); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The keySet is modifiable allowing remove so would allow removes without a lock.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea of the lock here is to only block, when a new delegate arrives or a delegate goes away/is replaced. If we lock any write operations, we will end up with deadlocks, when geenrated bundles are intialized.
| if (delegate == null) { | ||
| return backup.values(); | ||
| } else { | ||
| return delegate.values(); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The values is modifiable allowing remove so would allow removes without a lock.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea of the lock here is to only block, when a new delegate arrives or a delegate goes away/is replaced. If we lock any write operations, we will end up with deadlocks, when geenrated bundles are intialized.
| if (delegate == null) { | ||
| return backup.entrySet(); | ||
| } else { | ||
| return delegate.entrySet(); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The entrySet is modifiable allowing remove so would allow removes without a lock. Also the entries are modifiable via java.util.Map.Entry.setValue(V) so yet another way to modify the map.
| Collections.sort(knownReferences, | ||
| Comparator.comparingInt(this::getServiceRank).thenComparing(Comparator.reverseOrder())); | ||
| Collections.reverse(knownReferences); | ||
| if (knownReferences.isEmpty() && delegate != null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We compare in reverse order and then reverse the sorted result. That seems odd.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure, why I did that and it makes absolutly no sense... I should reverse the lists twice more, just to make sure :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| Collections.sort(knownReferences, | |
| Comparator.comparingInt(this::getServiceRank).thenComparing(Comparator.reverseOrder())); | |
| Collections.reverse(knownReferences); | |
| if (knownReferences.isEmpty() && delegate != null) { | |
| Collections.sort(knownReferences, | |
| Comparator.comparingInt(this::getServiceRank)); | |
| if (knownReferences.isEmpty() && delegate != null) { |
| public void start(BundleContext context) throws Exception { | ||
| if(getDefaultRegistryImplementation() != null && getDefaultRegistryImplementation() instanceof OSGiDelegatEPackageRegistry) | ||
| { | ||
| osgiEpackageRegistryTracker = new ServiceTracker<>(context, OSGiDelegatEPackageRegistry.FILTER, (ServiceTrackerCustomizer<EPackage.Registry, Object>) getDefaultRegistryImplementation()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not cast to OSGiDelegatEPackageRegistry to avoid an uncheck cast? It would be nicer to avoid calling getDefaultRegistryImplementation 3 times.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. I can change that.
| /** | ||
|
|
||
|
|
||
| private PlainOSGiBundleActivator plainOSGiBundleActivator; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand the current state correctly IS_ECLIPSE_RUNNING will be false we don't really need this case anymore. Or?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We would still need an additional Activator, in case IS_ECLIPSE_RUNNING is false and IS_OSGI_RUNNING is true. In this case, we need an activator but we don't have the EclipsePlugin. We could copy the stuff from the PlainOSGiBundleActivator in here as well, but we would end up with duplicated code.
|
@merks I again did not receive any notifications and thus missed your comments. I tried unsubscribe and then resubscribed. Lets see if, if it works better now. The last time we talked, you mentioned you wanted to take the PR and wrangle it, into something you like. Thus the question arises, if I should make the changes on the parts you have commented on? |
|
I'll try always @ mentioning you... Yes, could you please address the issues to some extent and then I will continue from that as the starting point... |
|
I've created the following PR that supersedes this PR: |
|
I'll close this as it's been superseded. |




The default EPackage Registry can be overwritten in mutlple ways (System property or ExtensionPoint). With this, if non of the other mechanisms apply, there now can be a service that acts as the static Registry.
The Mechanism makes sure that we will always have a functional and correctly filled static registry. All write commands, will be held in a backup registry, which will be reapplied if the service comes or goes. If multiple services are available, the one with the highest service ranking will win.
@merks I have a few tests for this, but I couldn't find tests in the Project, that have a running OSGi. If you want to have a look at the tests, they are here.