MyFaces Core
  1. MyFaces Core
  2. MYFACES-2995

Make method of determinine app context in FactoryFinder pluggable

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.0.3-SNAPSHOT
    • Fix Version/s: 2.0.5
    • Component/s: SPI
    • Labels:
      None

      Description

      As discussed on dev list.... geronimo would like to explicitly mark component boundaries rather than relying on the TCCL changing.

      1. MYFACES-2995.patch
        31 kB
        David Jencks
      2. MYFACES-2995-FactoryFinderProvider-1.patch
        23 kB
        Leonardo Uribe

        Issue Links

          Activity

          Hide
          David Jencks added a comment -

          Initial proposal for pluggable application context determination.

          Show
          David Jencks added a comment - Initial proposal for pluggable application context determination.
          Hide
          Leonardo Uribe added a comment -

          I thought a lot about FactoryFinder and how we can "extend" it. I'll do a resume, describing the problem in detail and providing some alternatives. This could be a little bit redundant, but it is necessary to gain a better understanding about what's going on and the direction to take.

          FactoryFinder is a class with three methods:

          public final class FactoryFinder
          {
          public static Object getFactory(String factoryName) throws FacesException

          {...}
          public static void setFactory(String factoryName, String implName) {...}

          public static void releaseFactories() throws FacesException

          {...}

          }

          The javadoc describe the intention of FactoryFinder class:

          "... FactoryFinder implements the standard discovery algorithm for all factory objects specified in the JavaServer Faces APIs. For a given factory class name, a corresponding implementation class is searched for based on the following algorithm...."

          On the javadoc there is more information from the point of view of the author (classloading stuff and so on), but just ignore it for a moment, because it only makes things confusing.

          In few words, this class allows to find JSF factory classes. The necessary information to create factory instances is loaded on initialization time, but which locations contains such information (for more information see JSF 2.0 spec section 11.4.2) (we are only interested in jsf factories initialization information) ?

          • Look factories on META-INF/services/[factoryClassName]
          • Look META-INF/faces-config.xml or META-INF/[prefix].faces-config.xml
          • Look the files pointed by javax.faces.CONFIG_FILES web config param (note WEB-INF/web.xml is taken into consideration)
          • Look the applicationFacesConfig on WEB-INF/faces-config.xml

          Based on the previous facts, the first conclusion to take into account arise: Configuration information is gathered per "web context". What is a "web context"? In simple terms, is the "space" where a web application is deployed. Let's suppose an EAR file with two WAR files: a.war and b.war. Both contains different "web applications" and when are deployed has different "web context", so both can provide different factory configuration, because both has different WEB-INF/web.xml and WEB-INF/faces-config.xml files.

          Now, given a request, how the web container identify a "web context"? At start, it receives the request information and based on that it decides which web application should process it. After that, it assign to a thread from is thread pool to be processed and the control is passed to the proper filters/servlets.

          So, if we don't have a servlet context/portlet context/whatever context, how to identify a "web context"? The answer is using the thread, but the one who knows how to do that is the web container, not the jsf implementation.

          The existing problem is caused by a "shortcut" taken to make things easier. Instead use the current "thread", it is taken as advantage the fact that each web application deployed has a different classloader. That is true for a lot of application servers, so the current implementation of FactoryFinder is based on that fact too and has worked well since the beginning.

          Now let's examine in detail how a "single classloader per EAR" option could work. If the EAR has two WAR files (a.war and b.war), we have two web context, and the initialization code is executed twice. When all FactoryFinder methods are called?

          • FactoryFinder.setFactory is called on initialization
          • FactoryFinder.releaseFactories is called on shutdown
          • FactoryFinder.getFactory is called after initialization configuration is done but before shutdown call to FactoryFinder.setFactory .

          Remember all methods of FactoryFinder are static.

          One possible solution could be:

          1. Create a class called FactoryFinderProvider, that has the same three method but in a non static version.
          2. A singleton component is provided that holds the information of the FactoryFinderProviderFactory. This one works per classloader, so the singleton is implemented using an static variable. To configure it, the static method should be called when the "classloader realm" is initialized, before any web context is started (the WAR is deployed). Usually the EAR is started as a single entity, so this should occur when the EAR starts, but before the WAR files are started (or the web context are created). The singleton will be responsible to decide which FactoryFinderProvider should be used, based on the current thread information.
          3. Add utility methods to retrieve the required objects and call the methods using reflection from javax.faces.FactoryFinder

          I provided a patch a patch for this one (MYFACES-2995-FactoryFinderProvider-1.patch)

          If no objections I'll commit this code soon.

          Show
          Leonardo Uribe added a comment - I thought a lot about FactoryFinder and how we can "extend" it. I'll do a resume, describing the problem in detail and providing some alternatives. This could be a little bit redundant, but it is necessary to gain a better understanding about what's going on and the direction to take. FactoryFinder is a class with three methods: public final class FactoryFinder { public static Object getFactory(String factoryName) throws FacesException {...} public static void setFactory(String factoryName, String implName) {...} public static void releaseFactories() throws FacesException {...} } The javadoc describe the intention of FactoryFinder class: "... FactoryFinder implements the standard discovery algorithm for all factory objects specified in the JavaServer Faces APIs. For a given factory class name, a corresponding implementation class is searched for based on the following algorithm...." On the javadoc there is more information from the point of view of the author (classloading stuff and so on), but just ignore it for a moment, because it only makes things confusing. In few words, this class allows to find JSF factory classes. The necessary information to create factory instances is loaded on initialization time, but which locations contains such information (for more information see JSF 2.0 spec section 11.4.2) (we are only interested in jsf factories initialization information) ? Look factories on META-INF/services/ [factoryClassName] Look META-INF/faces-config.xml or META-INF/ [prefix] .faces-config.xml Look the files pointed by javax.faces.CONFIG_FILES web config param (note WEB-INF/web.xml is taken into consideration) Look the applicationFacesConfig on WEB-INF/faces-config.xml Based on the previous facts, the first conclusion to take into account arise: Configuration information is gathered per "web context". What is a "web context"? In simple terms, is the "space" where a web application is deployed. Let's suppose an EAR file with two WAR files: a.war and b.war. Both contains different "web applications" and when are deployed has different "web context", so both can provide different factory configuration, because both has different WEB-INF/web.xml and WEB-INF/faces-config.xml files. Now, given a request, how the web container identify a "web context"? At start, it receives the request information and based on that it decides which web application should process it. After that, it assign to a thread from is thread pool to be processed and the control is passed to the proper filters/servlets. So, if we don't have a servlet context/portlet context/whatever context, how to identify a "web context"? The answer is using the thread, but the one who knows how to do that is the web container, not the jsf implementation. The existing problem is caused by a "shortcut" taken to make things easier. Instead use the current "thread", it is taken as advantage the fact that each web application deployed has a different classloader. That is true for a lot of application servers, so the current implementation of FactoryFinder is based on that fact too and has worked well since the beginning. Now let's examine in detail how a "single classloader per EAR" option could work. If the EAR has two WAR files (a.war and b.war), we have two web context, and the initialization code is executed twice. When all FactoryFinder methods are called? FactoryFinder.setFactory is called on initialization FactoryFinder.releaseFactories is called on shutdown FactoryFinder.getFactory is called after initialization configuration is done but before shutdown call to FactoryFinder.setFactory . Remember all methods of FactoryFinder are static. One possible solution could be: 1. Create a class called FactoryFinderProvider, that has the same three method but in a non static version. 2. A singleton component is provided that holds the information of the FactoryFinderProviderFactory. This one works per classloader, so the singleton is implemented using an static variable. To configure it, the static method should be called when the "classloader realm" is initialized, before any web context is started (the WAR is deployed). Usually the EAR is started as a single entity, so this should occur when the EAR starts, but before the WAR files are started (or the web context are created). The singleton will be responsible to decide which FactoryFinderProvider should be used, based on the current thread information. 3. Add utility methods to retrieve the required objects and call the methods using reflection from javax.faces.FactoryFinder I provided a patch a patch for this one ( MYFACES-2995 -FactoryFinderProvider-1.patch) If no objections I'll commit this code soon.
          Hide
          David Jencks added a comment -

          Since it's been about 2 1/2 months before this issue was really responded to I'd appreciate a few days to review your patch before you commit it. At the moment I don't understand how a per-ear object could possibly be of any use. IMO depending on the app server either a classloader based strategy as is used at present will work fine or the app server will need to use something else for every ear. There is little chance that one app server will use different classloading strategies for different ears and want to therefore use different code to distinguish jsf contexts for different ears.

          However I may have misunderstood your comments as I have not yet had a chance to look at your patch.

          Show
          David Jencks added a comment - Since it's been about 2 1/2 months before this issue was really responded to I'd appreciate a few days to review your patch before you commit it. At the moment I don't understand how a per-ear object could possibly be of any use. IMO depending on the app server either a classloader based strategy as is used at present will work fine or the app server will need to use something else for every ear. There is little chance that one app server will use different classloading strategies for different ears and want to therefore use different code to distinguish jsf contexts for different ears. However I may have misunderstood your comments as I have not yet had a chance to look at your patch.
          Hide
          Leonardo Uribe added a comment -

          The example of the EAR is just a "help" to understand the problem, but it is not a limitation. The important concept is FactoryFinderProviderFactory instance is an static var that should be initialized when the "classloader context" is initialized. For example, if I have 5 EAR files and all share the same myfaces implementation (by some specific classloading configuration done by the server), the container should initialize this instance before deploy/start any of the 5 EAR.

          I was thinking on make some changes on FactoryFinderProviderFactory, removing the code that set FactoryFinder._initialized to false. Instead, check if FactoryFinder has been initialized and if that so, log a warning indicating the instance will not be changed.

          I'll be pending of your review so we can close this issue.

          Show
          Leonardo Uribe added a comment - The example of the EAR is just a "help" to understand the problem, but it is not a limitation. The important concept is FactoryFinderProviderFactory instance is an static var that should be initialized when the "classloader context" is initialized. For example, if I have 5 EAR files and all share the same myfaces implementation (by some specific classloading configuration done by the server), the container should initialize this instance before deploy/start any of the 5 EAR. I was thinking on make some changes on FactoryFinderProviderFactory, removing the code that set FactoryFinder._initialized to false. Instead, check if FactoryFinder has been initialized and if that so, log a warning indicating the instance will not be changed. I'll be pending of your review so we can close this issue.
          Hide
          David Jencks added a comment -

          This looks ok to me although I don't like the amount of reflection that much. A few comments:

          • I don't see why FactoryFinderProvider is an abstract class rather than an interface.
          • I don't see why the api _FactoryFinderProviderFactory needs to know anything about FactoryFinderProviderFactory. Since you are already using reflection to poke around in the api, why not have FactoryFinderProviderFactory just set an object field in the api class directly?
          • I think I would make the tccl and optional replacement strategies more similar by putting the tccl based code in a FactoryFinderProvider like object set into _FactoryFinderProviderFactory by default.

          I haven't tried this with geronimo yet but I think it will work.

          Show
          David Jencks added a comment - This looks ok to me although I don't like the amount of reflection that much. A few comments: I don't see why FactoryFinderProvider is an abstract class rather than an interface. I don't see why the api _FactoryFinderProviderFactory needs to know anything about FactoryFinderProviderFactory. Since you are already using reflection to poke around in the api, why not have FactoryFinderProviderFactory just set an object field in the api class directly? I think I would make the tccl and optional replacement strategies more similar by putting the tccl based code in a FactoryFinderProvider like object set into _FactoryFinderProviderFactory by default. I haven't tried this with geronimo yet but I think it will work.
          Hide
          Leonardo Uribe added a comment -

          DJ >> This looks ok to me although I don't like the amount of reflection that much. A few comments:

          Well, it is unavoidable.

          DJ >> I don't see why FactoryFinderProvider is an abstract class rather than an interface.

          Good point. Yes, it could be an interface instead an abstract class, because it is not expected this one could change in the future. I'll do the necessary changes.

          DJ >> why not have FactoryFinderProviderFactory just set an object field in the api class directly?

          It is not possible to do that because it introduces a circular dependency between myfaces-api and myfaces-impl.

          DJ >> I think I would make the tccl and optional replacement strategies more similar by putting the tccl based code in a FactoryFinderProvider like object set into _FactoryFinderProviderFactory by default.

          Yes, it could be but I think it is better to let the code as is in the patch, because it is easy to compare the javadoc against the code, and it is clear if no FactoryFinderProvider implementation is provided, just do the default algorithm. Other reason to take into account is do not use reflection if it is not necessary.

          DJ >> I haven't tried this with geronimo yet but I think it will work.

          So can we close this issue as fixed? (after commit the code with some changed)

          Show
          Leonardo Uribe added a comment - DJ >> This looks ok to me although I don't like the amount of reflection that much. A few comments: Well, it is unavoidable. DJ >> I don't see why FactoryFinderProvider is an abstract class rather than an interface. Good point. Yes, it could be an interface instead an abstract class, because it is not expected this one could change in the future. I'll do the necessary changes. DJ >> why not have FactoryFinderProviderFactory just set an object field in the api class directly? It is not possible to do that because it introduces a circular dependency between myfaces-api and myfaces-impl. DJ >> I think I would make the tccl and optional replacement strategies more similar by putting the tccl based code in a FactoryFinderProvider like object set into _FactoryFinderProviderFactory by default. Yes, it could be but I think it is better to let the code as is in the patch, because it is easy to compare the javadoc against the code, and it is clear if no FactoryFinderProvider implementation is provided, just do the default algorithm. Other reason to take into account is do not use reflection if it is not necessary. DJ >> I haven't tried this with geronimo yet but I think it will work. So can we close this issue as fixed? (after commit the code with some changed)
          Hide
          Leonardo Uribe added a comment -

          Ok, I'm closing it as fixed, since I already did the changes suggested.

          Show
          Leonardo Uribe added a comment - Ok, I'm closing it as fixed, since I already did the changes suggested.

            People

            • Assignee:
              Leonardo Uribe
              Reporter:
              David Jencks
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development