-
-
Notifications
You must be signed in to change notification settings - Fork 301
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
Facades #5900
Facades #5900
Conversation
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.
Thanks a lot for this, @pkriens! Unfortunately I am going on a big road trip over the next couple of weeks and I will not have a lot of time to look at this in the detail that it probably deserves until mid December. Here is a first pass.
I think it would be helpful for me to attempt to summarise the main differences between this implementation and the implementation in #5036, to see if I have understood it correctly. At a high level, the main changes are:
- abstracts the various
ServiceTracker
implementations that the current facades implement and factor out the common code (this is whatBinder
does, in conjunction with theFacadeManagerImpl
). - moves all of this facade-handling/dynamics code into its own separate bundle,
- moves all of the facade implementations into their own bundle,
- adds a background task to cleanup facades whose backing objects have gone (instead of relying on synchronous calls to
ServiceTracker
methods to do this check). - adds state-management capability so that as backing implementations objects come-and-go they can re-create their state from their predecessor.
Please let me know if I've missed something important. This list of itself is still a significant improvement over what we already had in #5036, which contained classes that were largely cut-and-pastes of each other.
One thing that seems to be missing in this PR (when compared with the existing implementation) is the dynamic proxy creation of facades for EPs that are plain interfaces. This removes the need for custom facade classes (like the IncrementalProjectBuilderFacade
and ClasspathContainerInitializerFacade
), but it obviously only works for interfaces. Forgive me if it is there and I've missed it somehow. It would belong in bndtools.facades
somewhere.
@@ -0,0 +1,28 @@ | |||
#-runfw: org.apache.felix.framework;version=5 |
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 this .bndrun
file intended to do something useful, or has it snuck into the commit by mistake? If it does something useful, perhaps a one or two-line comment at the top explaining what its use is.
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 just found that this launch file is used by the Launchpad test. A single line comment to explain that would make me happy. :smile;
* {@link Delegate#create(String, Object)} when it needs to be recreated. | ||
*/ | ||
@Nullable | ||
Object getState(); |
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.
Do you think it might be useful to parameterise the return type? The erasure would be the same but concrete implementations would be able to access their state object in a type-safe way.
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 comfortable doing this ... the getState() is coming from another object than setState(). The implementor of Memento must really inspect the received object. It is not possible to guarantee the type here, so I'd rather not give the impression.
biz.aQute.bnd.facade.provider/src/biz/aQute/bnd/facade/provider/FacadeManagerProvider.java
Show resolved
Hide resolved
biz.aQute.bnd.facade.provider/test/biz/aQute/bnd/facade/provider/FacadeManagerProviderTest.java
Show resolved
Hide resolved
biz.aQute.bnd.facade.provider/test/biz/aQute/bnd/facade/provider/FacadeManagerProviderTest.java
Show resolved
Hide resolved
|
||
@ConsumerType | ||
public class IClasspathContainerFacade extends EclipseBinder<IClasspathContainer> | ||
implements IClasspathContainer { |
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.
IClasspathContainer
is not an Eclipse extension point. The extension point is ClasspathContainerInitializer
, which is (among other API features) a factory that creates the IClasspathContainer
instances via the initialize()
method. So I think this needs to change.
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.
Thanks. Here I need the help :-)
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 will see if I can copy the facades you already made.
* each EP type, e.g. IClassPathContainer, has a corresponding facade. E.g. | ||
* IClassPathContainer (see bndtools.facades project). |
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.
As noted elsewhere, IClasspathContainer
is not an EP - ClasspathContainerInitializer
is. Just making a note here so that when we fix the code we don't forget the 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.
I think I need some help with the actual facades. I've made an IAdapterFactory example. How we do the actual facades is something I need your help with.
--- Signed-off-by: Peter Kriens <[email protected]> Signed-off-by: Peter Kriens <[email protected]>
--- Signed-off-by: Peter Kriens <[email protected]> Signed-off-by: Peter Kriens <[email protected]>
--- Signed-off-by: Peter Kriens <[email protected]> Signed-off-by: Peter Kriens <[email protected]>
--- Signed-off-by: Peter Kriens <[email protected]> Signed-off-by: Peter Kriens <[email protected]>
--- Signed-off-by: Peter Kriens <[email protected]> Signed-off-by: Peter Kriens <[email protected]>
9e4ce34
to
ba6de7c
Compare
I've just pushed the function to create facades with the -generate function. This works extremely well although it was complicated because there are facades that extend classes, excluding dynamic proxies. The facade can also sometimes require the implementation of optional interfaces. The code generation creates an interface for the delegate service. This interface declares all methods of the class (if any) and all interfaces. This removes the requirement that the service needs to extend a class. It then creates a class Delegate and provides the delegating to the Binder implementation. The base class of the surrounding class is parameterizable because it can be different for different environments. I then also created an IAdapterFactory example. I use the IExecutableExtensionFactory and IExecutableExtension to let the extension point specify the facade.id service property. Quite a lot of code but it looks very nice imho. |
Please don't do that, simply register it and it will be picked up, once is merged it can be as simple as |
But they do it incorrect as I tried to explain some time ago. First try to understand the very complex and dynamic nature of OSGi before you comment. The Adapter framework in Eclipse is a 3 year old kindergarten solution :-( And yes, I know it works for you. |
You are mixing things up here ...
I do understand that and the
Not everything you don't understand is a "kindergarten solution" |
You might want to lookup Dunning–Kruger ... |
@kriegfrj what do you think, I shall I commit this code? We can then take it from there. I think it will really make the facades more reliable and easier to implement? |
Don't think I will have time to work on this anymore. |
@kriegfrj has been working on using the facade pattern for Extension Point (EP) classes. This will allow bndtools code to use Declarative Services (DS) components to provide the executable class for EPs.
This is a proposal to provide central support for this pattern.
The
biz.aQute.bnd.facade.api
package defines a service for facades. The Binder class provides an object that links a facade class to a backing service. A facade class is a class that delegates all methods to the Binder, each EP type, e.g. IClassPathContainer, has a corresponding facade. E.g. IClassPathContainer (see bndtools.facades project). The facade type and the backing service type might differ. For example, the IncrementalProjectBuilder has many final methods. A special interface is necessary that passes the IncrementalProjectBuilder as a parameter to the methods that the backing service should provide.The Binder object is linked via statics (yuck) to a Facade Manager. The Facade Manager tracks delegate services.A delegate service can either be a PROTOTYPE component service or implement the Delegate interface. When there is a proper delegate, a Binder is bound to that delegate. Both Binder and delegate have a FACADE_ID which is used to line them up. Multiple Binder objects can be bound to a single delegate. Each Binder object will be bound to a unique instance.
The EP defines the name of the class. To define the FACADE ID, the class name is appended with a ':' and the facade id. The Eclipse Binder class in bndtools.facade project handles the interaction with the EP subsystem.
When a Facade Manager is activated, it will set itself as manager in the Binder class. This will register each Binder that was created so far and not yet purged. When a delegate is registered, the Facade Manager will take care that the Binder's with the same type are bound. If the service is unregistered, it will unbind the Binder and close the instance.
A Binder keeps a weak reference to the facade object. If the facade object is garbage collected, the Binder should be closed. A periodic task in the Facade Manager is responsible for this.
When a Binder is unbound, the backing service can provide a memento if it implements the Memento interface. When a new instance is created, this memento is handed over to the create function in Delegate. Clearly this state should not contain references to classes from the backing service's bundle since the bundle is likely restarted. However, this allows backing services to transfer state between a current instance and a future instance.