Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.
Bug 206936 - [registry] Improving registry notification mechanism
Summary: [registry] Improving registry notification mechanism
Status: RESOLVED FIXED
Alias: None
Product: Equinox
Classification: Eclipse Project
Component: Compendium (show other bugs)
Version: 3.4   Edit
Hardware: PC Windows XP
: P3 enhancement (vote)
Target Milestone: 3.4 M4   Edit
Assignee: equinox.compendium-inbox CLA
QA Contact:
URL:
Whiteboard:
Keywords:
: 130655 193550 203288 (view as bug list)
Depends on:
Blocks:
 
Reported: 2007-10-19 14:40 EDT by Oleg Besedin CLA
Modified: 2007-11-12 11:53 EST (History)
5 users (show)

See Also:


Attachments
Draft patch for part (A) (26.82 KB, patch)
2007-10-19 15:00 EDT, Oleg Besedin CLA
no flags Details | Diff
Patch implementing new registry listener (part A) (47.70 KB, patch)
2007-10-25 11:38 EDT, Oleg Besedin CLA
no flags Details | Diff
New registry listener (part A) plus illustration of (B) (69.75 KB, patch)
2007-11-01 17:19 EDT, Oleg Besedin CLA
no flags Details | Diff
Patch - new registry listener only, plus JUnits and Javadocs (53.43 KB, text/plain)
2007-11-12 11:07 EST, Oleg Besedin CLA
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Oleg Besedin CLA 2007-10-19 14:40:27 EDT
This bug is opened as a central place to work on improvement to the registry notification system. 

What is not peachy with the current notification system:
- Can not see what exactly has been modified (bug 130655)
- Dynamic behavior is hard to work with (bug 115257)
- Not designed to work with changes to extension points (bug 203288)
- Consistency of information presented in notifications (bug 185977)

The "consistency" requirement can't be helped much at this point (more to follow) but can be mitigated to an extent by leading users to a different usage patterns ("simple" and "smart" caches, more to follow).
Comment 1 Oleg Besedin CLA 2007-10-19 14:42:15 EDT
Constraints

At this point we can't force people to change the way extension registry is used. The listeners to the current notification system have to continue receiving same information as they are today in the same form. Which in particular forces the following constraints:

- registry notifications have to be sent asynchronously. 
The current implementers and UI in particular, assume asynchronous nature. That can not be changed without breaking existing implementations. In turn, if we were to send "new" notifications synchronously and "old" notifications asynchronously, sequence of events would become unpredictable. Hence, new notifications have to remain asynchronous.

- old listeners can't receive notifications on addition/removal of extension points
Those events would be unexpected and will contain empty delta lists which listeners might not anticipate. In fact, doing so breaks even our own JUnits tests.

- registry notifications can't rely on extension IDs
The IDs on extensions are optional and can not be relied upon. Most people outside of SDK don't use IDs on extensions. Alternatively, some implementers adopted schemas with IDs stored in configuration elements. In hindsight, IDs on extensions should have been made mandatory, but we can't force this change now. As a result, registry notifications can't rely on extension IDs to provide unique (or meaningful) identifications.
Comment 2 Oleg Besedin CLA 2007-10-19 14:46:20 EDT
Existing use cases

After studying usage of registry notifications in SDK, tools, and technology projects, few common trends have emerged.

Listeners reacted to one of:
(a) events related to specific extension point [this is the majority of uses], or
(b) all events [listeners in this group were tooling related - such as PDE registry, registry viewer].

In all cases listeners were registered to update user caches elements of which corresponded to registry elements. Most of the time processing followed one of the following patterns:
(1) "simple" cache. In this case on any event user's cache corresponding to the registry objects was invalidated. Then cache was recreated either lazily or immediately. This pattern was used in the majority (70%) of uses.
(2) "smart" cache.  In this case only a single element in the cache corresponding to the IExtension was added/removed (25% of use cases).
(3) In a few cases (5%) more complicated processing was used. It is likely that those cases could boil down to (2) but it would be nice to keep a "generic" mechanism for cases that are more complicated than (1) or (2).

Conclusions from usage patterns:
- listeners should be able to register on either a specific extension point or on all extension points (let's call them "global" listeners)
- all extension changes related to a registry event shall be passed in one method call (otherwise non-lazy "simple" caches would have to recreate local every caches for every single extension modified)
- (1), (2), and (3) represent very different uses that might be best served by different mechanisms:
	(3) - new notification type passing in only directly modified extensions and extension points to be added in addition to existing registry notification (see Constraints)
	(2) - a user would create a special tracker type (aka ExtensionTracker2 / ServiceTracker). The implementation will be based on the implementation of (3)
	(1) - create an extension point on the registry that would allow user to "watch" extension point. Then extension point is modified, user class will get notify() call with no arguments (only if user's bundle was activated by this time?)

So the plan is:
(A) add a new listener type (it will be notified when extension point is added/removed as well as extension) that can be registered on an extension point or "globally". Such listener will be provided with exact extensions and extensions points being added/removed (and only those directly modified will be included in the notification)
(B) based on the new listener, create convenience mechanism aka ExtensionTracker2 / ServiceTracker
(C) create an extensions point to "watch" an extension point to provide a very simple notification mechanism

Question is: would be it overkill to have 3 mechanisms? (A) is probably a must have and base for the rest of work; (C) seems like a simple way for people to jump on a "dynamic" train without too much coding (after all, for most people registry events are infrequent so no point in complicating their lives). (B) is was requested in bug 115257.

But that would create quite a few APIs: at least 3 new interfaces; at least 5 new methods on IExtensionRegistry, and a new extension point.

Comment 3 Oleg Besedin CLA 2007-10-19 15:00:21 EDT
Created attachment 80793 [details]
Draft patch for part (A)

Patch applies to equinox.registry and equinox.common.

This is a draft patch to illustrate implementation of the new registry event listener. The patch needs to have JUnits added and lots of testing. 

Places of interest:

=> API front:
added IRegistryEventListener with callback methods
	added(IExtension[] / IExtensionPoint[] )
	removed(IExtension[] / IExtensionPoint[] )

IExtensionRegistry gets new methods
	addListener(IRegistryEventListener listener)
	addListener(IRegistryEventListener listener, String extensionPointId)
	removeListener(IRegistryEventListener listener)

ListenerList (from equinox.common) gets
	removeAll(Object listener)

=> Implementation / SPI 
This was particularly interesting due to the fact that scheduling of registry events is exposed via SPI RegistryStrategy. To work around it, new listeners and new combined delta are passed together with "original" ones and unpacked when events are processed. This creative approach allows us to keep SPIs unmodified.
Another advantage from bundling old and new notifications together is that same temporary object manager is used (and thus reducing overheads and potential problems associated with having different types of deltas).

Pascal, Tom, could you have a look at this?

(I expect implementations to come in stages - (A) first, then based on it (C) and (B). So for now this deals only with new listeners, parts for trackers and "watching" of extension points will come later.)
Comment 4 Oleg Besedin CLA 2007-10-19 15:04:06 EDT
*** Bug 193550 has been marked as a duplicate of this bug. ***
Comment 5 Thomas Watson CLA 2007-10-23 17:56:49 EDT
Overall I like the new IRegistryEventListener idea.  It seems like a more simple API than the original IRegistryChangeListener/IRegistryChangeEvent/IExtensionDelta model.  The existing mechanism (IExtensionDelta) had the ability to get the IExtensionPoint for the affected extension.  Is the method IExtension.getExtensionPointUniqueIdentifier() good enough in the new IRegistryEventListener to get the affected IExtensionPoint?
Comment 6 Oleg Besedin CLA 2007-10-24 10:22:03 EDT
Thanks Tom; yes, that is correct. From the search of the existing code, most listeners (aside from tooling) would be registered using #addListener(..., extensionPointID). Receiving notification on such listener would imply that this notification is for the extension point with ID specified - thus eliminating some extra checks from user's code. 

==> Deep dive - consistency of information presented in notifications:

There is a slight but interesting difference under the hood in a way how IExtensionPoint could be retrieved in this case. The "new" notifications eliminate one of potential inconsistencies of information in links between extensions and extension points.

In the scenario where a bundle containing both an extension point and its extensions is uninstalled, the "old" deltas include both extension point and extensions via temporary object manager. The extension point in this case is provided as IExtensionPoint, but it is unlinked from extensions so you can't get extensions from it. This presents a consistency problem: extensions say they belong to this extension point, but extension point is no longer associated with them.

With the "new" deltas you'll get a string ID for the extension point, but to get IExtensionPoint you'd have to go through the regular registry manager that, in the scenario above, will return null as the extension point has been deleted from the "regular" registry before notification is sent out.

So, in the scenario above, "new" deltas present slightly more consistent view of the registry. The full "consistency" of information can not be achieved with asynchronous notifications (in a practical way), but every little bit helps.
Comment 7 Oleg Besedin CLA 2007-10-25 11:38:47 EDT
Created attachment 81171 [details]
Patch implementing new registry listener (part A)

Updated first patch:
- Added JUnits
- Fixed bug with global listeners
- Adjusted order of callbacks
- Change to ListenerList no longer needed

Patch applies to org.eclipse.equinox.registry and org.eclipse.core.tests.runtime.
Comment 8 Matt Lavin CLA 2007-10-25 13:06:22 EDT
I was debugging a problem with my extension reader the other day and I noticed something that be a possible improvement.  I've tried to double check that it wasn't already discussed in the comments above but forgive me if I missed it.

I found that if plugins are dynamically installed into the OSGi environment that the notification from the extension registry does not always happen in the order I would expect.  In my testcase I had two plugins, A and B, where B was dependent on A.  Both plugins had plugin.xml files and when they were both installed and started it was possible to see registry events about B before the events for A came in.  This caused trouble when extension points in B referenced things in A (B does depend on A after all).  

Does this behavior make sense, or could it be added to the list of possible improvements?  Ie, could extensions for bundles be read (and events sent) in dependency order?
Comment 9 Thomas Watson CLA 2007-10-25 13:15:06 EDT
(In reply to comment #8)
This is probably because of the order of the BundleEvent.RESOLVED events being fired by the Framework.  The framework does not order these events in any particular order.  The registry receives the events one by one and processes the plugin.xml for the bundle and fires the registry events.  If the RESOLVED events are not fired in the order you expect then the registry events will not be either.  One possibility would be for the registry to batch all the registry events.  This should be possible with the BatchBundleListener in the equinox framework.  Then the registry could pay attention to batchBegin/batchEnd calls to determine if the events could be batched into one single registry event after all the RESOLVED events have been fired.

Comment 10 Matt Lavin CLA 2007-10-25 13:27:41 EDT
(In reply to comment #9)
You are exactly right about the reason being the order of BundleEvents.  I saw exactly that behavior (the order of the registry events matches the order of the BundleEvents).

Having the events come in an order other than the dependency order can make implementing extension readers tricky (ie, the registry events become almost useless) if the extension reader has dependencies on other extensions in it's dependencies.

Ultimately, we'll probably have to come up with a solution to the problem before you get the improvements into Equinox, but anything that can be done to normalize the order would help future developers.
Comment 11 Thomas Watson CLA 2007-10-25 13:38:45 EDT
(In reply to comment #10)
> Ultimately, we'll probably have to come up with a solution to the problem
> before you get the improvements into Equinox, but anything that can be done to
> normalize the order would help future developers.
> 

Not sure what you mean.  The Framework could sort the events by bundle dependency order, but I think that is at best a partial solution to the problem.  Dependencies from extension/extension-point relationships are not understood at the framework level.  For example, you could have Bundle X depend on Bundle Y (by Require-Bundle or Import-Package) and then have Bundle Y provide an extension to an extension point in Bundle X.  Nothing prevents this kind of relationship.

Here we have a classloader dependency with X->Y and a registry dependency with Y->X.  In this case I think it would be best if the registry could batch the RESOVLED events from X and Y.  Once both have been processed then the registry could send out one event to the listeners that contains all the registry changes from both X and Y.
Comment 12 Matt Lavin CLA 2007-10-25 13:46:57 EDT
(In reply to comment #11)
> Here we have a classloader dependency with X->Y and a registry dependency with
> Y->X.  In this case I think it would be best if the registry could batch the
> RESOVLED events from X and Y.  Once both have been processed then the registry
> could send out one event to the listeners that contains all the registry
> changes from both X and Y.

You are right again.  The problem is really tricky (harder than I originally pictured).  In our case we are pretty sure that the extension dependencies match the classloading dependencies, but you are right when you say it doesn't have to be that way.  
Comment 13 Oleg Besedin CLA 2007-10-25 14:21:05 EDT
Matt, at least in theory the scenario you describe is supported by the extension registry. If bundle with extensions is resolved before the extension point is added, such extensions are internally marked as "orphans" and no event is send. When extension point is added, an event is send containing the new extension point and previously "orphaned" extensions. 

However, when you register a listener, typically it is done with a filter based on namespace. In your scenario extensions [usually] would reside in a different namespace from the extension point - so you might have to fight the filter to get both extensions and extension point. 

At least this is how I think it supposed to work; if you find this not to be the case with the current notification mechanism be sure to file a bug - hopefully with a reproducible test case. 

And for the new listener, filtering is done by extension point ID so filter should not be a problem; I'll check if orphaned extensions still work with the new listener - that's a great suggestion, thank you for it.
Comment 14 Matt Lavin CLA 2007-10-25 14:27:48 EDT
(In reply to comment #13)

Our situation isn't quite as simple as the extension point is defined in one bundle and other extends it.  We have two different extensions were one makes a reference to the other by id.  In our extension reading code as assume that the extension referenced by id is already there because it is in a plugin that it depends on, but that isn't true if the extensions are ONLY processed through events.  Ie, what our reader code needs to do is when it is asked for an extenion point by id it will first see if it's gotten an event about it and then search the registry (just to make sure that the event hasn't already been processed and the event is pending).  The other solution is to implement an orphan strategy like you have, but we'll see which one is easier.
Comment 15 Oleg Besedin CLA 2007-10-25 15:57:57 EDT
On Tom's advice I opened bug 207505 to capture scenrio expressed in the comment 14.
Comment 16 Oleg Besedin CLA 2007-11-01 17:19:33 EDT
Created attachment 81886 [details]
New registry listener (part A)  plus illustration of (B)

This patch has the implementation of the new registry listener very similar to the last one and adds an illustration for a possible tracker mechanism - see ExtensionBackedMap for API front and EclipseAppContainer for an example of usage. 

(The code for the tracker mechanism (part B) obviously needs cleanup, it is here to illustrate a possible implementation.)

Couple points on "convenience" mechanisms (parts B and C) now that I had a chance to play with implementation details:

- Part C ("watcher" extension point) would have to be scrapped. There is no mechanism to declaratively describe *instance* of a class to receive notifications. While this could be worked around, the amount of code and new concepts would be way overkill for a "nice to have" convenience feature. An interesting outcome of this is that we could benefit from having a generic mechanism to register listeners in a "declarative" way. I'll post a message on this on the development mailing list and we'll see where it goes from here

- the Part B - tracker. I played with 2 designs: one hiding underlying cache structure and one exposing it as a java.util.Map. The tracker hiding cache seems to be too restrictive while the tracker that expends Map becomes a bit complicated to grasp for a "convenience" thing. At this point I am really not sure if it provides convenience or inconvenience: tracker helps save a few lines of code but introduces new concepts. Any comments on usability of the tracker will be highly appreciated.

The bottom line:
- I still very much like the new listener
- Convenience mechanisms don't seem to offer all that much, any thoughts?
- A generic mechanism to have listeners registered in a "declarative" way would be very useful
Comment 17 Thomas Watson CLA 2007-11-02 09:07:40 EDT
I liked my initial review of the API in comment 5.  But I'm concerned about adding another API for which nobody will use.  If we move forward with the new API what plans do we have to get the community to move up?  Would we try to "force" the Eclipse Platform team to move up?  Would we deprecate the old APIs?
Comment 18 Oleg Besedin CLA 2007-11-12 11:07:14 EST
Created attachment 82667 [details]
Patch - new registry listener only, plus JUnits and Javadocs

From the discussion and trial implementations, it seems that "convenience" mechanisms don't add much here. So here is the cleaned patch with new listener, Javadocs, and JUnits. 

The new API methods get "provisional" note saying that they can be modified or removed before 3.4 is finalized. The "old" methods are not officially deprecated at this point (if "old" methods work for existing implementations, no need to switch), but are getting notices suggesting to use new methods for new implementations.

Instead of putting "convenience" mechanisms into SDK, I'll write an article on the use cases we found with some recommended approaches; most likely code from the previous patch (ExtensionBackedMap) will be included in the article.

(By the way, if you'd like to add to or review the article on the "using registry on a dynamic environment" be sure to let me know.)
Comment 19 Oleg Besedin CLA 2007-11-12 11:46:16 EST
Patch applied to CVS head.

Future developments:
- Write an article on using extension registry in a dynamic environment
- "Provisional" API notices to be removed before M5
- For Eclipse 4.0, see if we can create a declarative listener registration on top of EventAdmin
Comment 20 Oleg Besedin CLA 2007-11-12 11:52:53 EST
*** Bug 203288 has been marked as a duplicate of this bug. ***
Comment 21 Oleg Besedin CLA 2007-11-12 11:53:52 EST
*** Bug 130655 has been marked as a duplicate of this bug. ***