Hello Bernd,
Thanks for looking into this. I think geronimo might be able to
work
with the changes you have made, but I don't think it would be a
good
idea in the current form. I have two suggestions.
1. Please make use of the discovery mechanism optional. Geronimo
controls the startup order of all these components and it is much
simpler to simply create and install the instances of the
components
at he appropriate time during startup rather than hiding the
information about what class (not instance) will be used
somewhere in
the structure of the classloading hierarchy and then trying to fish
it out again through some procedure that takes at least a full page
to explain. I feel strongly enough about this that I would rather
use setAccessible on private members of the discovery classes to
install components than to use it as it is intended. Also, in its
current form geronimo is getting exceptions from the discovery code
on shutdown and its possible that the changes have significantly
slowed startup.
2. While I think adding a newInstance method to AnnotationProcessor
is a step forward, I would still prefer that myfaces use an
interface
like the one I suggested with only
newInstance
and
destroyInstance
methods, and provided an adapter to the AnnotationProcessor
interface. If geronimo implements the AnnotationProcessor
interface
itself, we will do all the work implied by the newInstance,
processAnnotations, and postConstruct methods in the newInstance
method. While right now this won't cause any problems according to
my analysis of the MyFaces code currently using the
AnnotationProcessor interface, it makes it very easy for future
developers to insert required functionality between MyFaces
calls to
these methods. If these calls to newInstance, processAnnotations,
and postConstruct are all in an adapter class if is very easy to
find
and compensate for any changes in this area.
I've attached a further patch MYFACES-1559-3.patch with suggestions
for these changes to the jira issue.
i'll mention that I think it might simplify the structure, ease of
understanding, and speed of myfaces if the LifecycleProvider
instances used by ManagedBeanFactory and the listeners were
supplied
to their constructors and held in final variables. This would
require that there were instances of ManagedBeanFactory and the
listeners for each application deployed. Despite my interest in
this
I don't have time or sufficient understanding of myfaces to try to
implement it at this time. This is similar to my desire to have
geronimo directly set the LifecycleProviderFactory/
AnnotationProcessorFactory rather than having the factory
examine its
environment for something that might work.
many thanks!
david jencks
On Mar 12, 2007, at 6:06 PM, Bernd Bohmann wrote:
> Hello David,
>
> we can move the AnnotationProcessor to the package
> org.apache.myfaces or an other package and add the method
>
> Object newInstance(String className);
>
> to the interface.
> (I like the idea for possible constructor dependency injection)
>
> And we should lookup the AnnotationProcessorFactory with
> JDK1.3-style service discovery.
>
> http://jakarta.apache.org/commons/discovery/apidocs/org/apache/
> commons/discovery/tools/DiscoverSingleton.html
>
> With this service discovery you can add your
> ApplicationIndexedAnnotationPrcessorFactory
>
> Just commited my changes based on your idea.
>
> Regards
>
> Bernd
>
> David Jencks wrote:
>> There's been a lot of discussion about annotation processing
in a
>> long thread http://www.nabble.com/%40PreDestroy%2C-Servlet-API
%2C-
>> tf3284592.html#a9136472 The current state of the code is that
>> managed objects are created by MyFaces code, and then fed to an
>> annotation processor using an interface like:
>> public interface AnnotationProcessor {
>> public void postConstruct(Object instance);
>> public void preDestroy(Object instance);
>> public void processAnnotations(Object instance);
>> }
>> (Exceptions removed for clarity)
>> I have been implementing annotation support in the geronimo app
>> client container and the geronimo-jetty6 integration and
studying
>> the openejb3 and native jetty annotation support and am starting
>> to look at annotation support in a geronimo-myfaces integration,
>> and have some ideas about how I'd like to handle geronimo
>> injecting dependencies into jsf managed beans.
>> I'd like to propose that MyFaces use an interface like this for
>> dealing with managed object construction, dependency injection,
>> and lifecycle methods:
>> public interface LifecycleProvider {
>> Object newInstance(String className);
>> void destroyInstance(Object o);
>> }
>> This would fit in well with how annotation processing/dependency
>> injection is done in the rest of geronimo. It also would let
the
>> container in which MyFaces is running supply additional features
>> such as supporting constructor dependency injection.
>> To go into what is probably blindingly obvious detail, this
would
>> be a MyFaces interface and the container in which MyFaces is
>> running would supply an object implementing this interface for
>> each application.
>> It's more or less trivial to write an adapter between this
>> interface and the AnnotationProcessor interface currently in
use,
>> for integration with containers that want to supply an
>> AnnotationProcessor.
>> So far I've thought of two issues, IMO one minor and the other
>> requiring more thought (at least on my part :-). Also I'm
not at
>> all familiar with the jsf spec so it's entirely possible I'm
>> proposing details that can't be implemented.
>> 1. The current code looks for ManagedBeanBuilder.NONE in between
>> injecting dependencies and calling postConstruct. I don't think
>> this is appropriate: I think whatever is handling the
annotations
>> should know not to call postConstruct for this class. If
however
>> the same class can be used in several different scopes the
>> newInstance method could take the ManagedBean as parameter
instead
>> of the class name.
>> 2. I'm proposing that the container inject an instance of
>> LifecycleProvider for each jsf application. This leads to the
>> question, injects into what? One simple possibility is a
>> singleton LifecycleProviderFactory that indexes
LifecycleProvider
>> by application classloader. However I wonder if there is a
way to
>> more directly inject the LifecycleProvider into the parts of
>> MyFaces that actually need to use it rather than making these
>> components go fishing for the one they need. The kind of lazy
>> initialization currently in wide use requires a lot more
>> synchronization than it currently has to work reliably. I would
>> prefer to use constructor dependency injection to final
fields to
>> avoid this kind of problem.
>> I've opened a jira issue to hold code samples related to this
>> proposal, and attached some initial implementations of these
ideas
>> for discussion. Right now these new classes aren't hooked up to
>> MyFaces, although I plan to work on that next.
>> Initial classes:
>> A core/impl/src/main/java/org/apache/myfaces/config/
>> annotation/LifecycleProviderFactory.java abstract class for
>> singleton factory.
>> A core/impl/src/main/java/org/apache/myfaces/config/
>> annotation/ApplicationIndexedLifecycleProviderFactory.java a
>> LifecycleProviderFactory that expects to be populated by an
>> external framework, with one LifecycleProvider instance per
>> application classloader.
>> A core/impl/src/main/java/org/apache/myfaces/config/
>> annotation/LifecycleProviderToAnnotationProcessorAdapter.java an
>> adapter between the LifecycleProvider implementation I'm
proposing
>> and the existing AnnotationProcessor interface currently in use.
>> This basically relies on there being only one
AnnotationProcessor
>> shared between all applications. This matches the current
>> implementation but I think it is unsatisfactory in general.
>> A core/impl/src/main/java/org/apache/myfaces/config/
>> annotation/LifecycleProvider.java Proposed interface for MyFaces
>> to plug in external services that handle annotations, object
>> construction, etc.
>> A core/impl/src/main/java/org/apache/myfaces/config/
>> annotation/AnnotationProcessorLifecycleProviderFactory.java a
>> LifecycleProviderFactory that uses the
>> LifecycleProviderToAnnotationProcessorAdapter.
>> The jira issue is https://issues.apache.org/jira/browse/
MYFACES-1559
>> Comments? Flames?
>> many thanks,
>> david jencks
>
