Uploaded image for project: 'Struts 2'
  1. Struts 2
  2. WW-4793

Don't add JBossFileManager as a possible FileManager when not on JBoss

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Trivial
    • Resolution: Fixed
    • Affects Version/s: 2.5.10
    • Fix Version/s: 2.5.12
    • Component/s: Core
    • Labels:
      None

      Description

      When the application starts and there is no FileManager specified as initParam, the JBossFileManager gets added.

      This results in the check happening everytime a FileManager is requested.
      (When turning on logging, i can see it happening 12 times when starting a simple application)

      org.apache.struts2.dispatcher.Dispatcher
      ...
          private void init_FileManager() throws ClassNotFoundException {
              if (initParams.containsKey(StrutsConstants.STRUTS_FILE_MANAGER)) {
                  ...
              } else {
                  // add any other Struts 2 provided implementations of FileManager
                  configurationManager.addContainerProvider(new FileManagerProvider(JBossFileManager.class, "jboss"));
              }
              ...
          }
      ...
      
      com.opensymphony.xwork2.util.fs.DefaultFileManagerFactory
          private FileManager lookupFileManager() {
              Set<String> names = container.getInstanceNames(FileManager.class);
              ...
              for (FileManager fm : internals) {
                  if (fm.support()) {
                      return fm;
                  }
              }
              return null;
          }
      

      My suggestion would be to not add it if it's not supported.

      I don't know what the best way to do this would be.

      The possibility i was thinking of so far involves the following:

      • Creating a seperate utility class to check if it can support it
        • perhaps a public static innerclass of JBossFileManager with public static method(s) to check it?
        • or a seperate class (although i think this might be awkward)
      • adding a test around adding the JBossFileManager to only do it when it could be supported.

      additional information:

      • log messages were noticed by adjusting the log4j2 configuration in the form-processing application from struts-examples and starting it.

        Issue Links

          Activity

          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user sdutry opened a pull request:

          https://github.com/apache/struts/pull/136

          WW-4793 only add JBossFileManager when supported

          You can merge this pull request into a Git repository by running:

          $ git pull https://github.com/sdutry/struts WW-4793

          Alternatively you can review and apply these changes as the patch at:

          https://github.com/apache/struts/pull/136.patch

          To close this pull request, make a commit to your master/trunk branch
          with (at least) the following in the commit message:

          This closes #136


          commit f7beeacf400ec6bdb27a7d67deb3b55dcc0f5dac
          Author: Stefaan Dutry <stefaan.dutry@gmail.com>
          Date: 2017-05-01T05:54:19Z

          added utility innerclass for checking support

          commit 4be974a482a2894b677ee4d2292ec778dfcd429e
          Author: Stefaan Dutry <stefaan.dutry@gmail.com>
          Date: 2017-05-01T06:00:54Z

          only add JBossFileManager when it's supported

          commit b140218cf2c7c634a6c42cd859d6d3b81cb5c290
          Author: Stefaan Dutry <stefaan.dutry@gmail.com>
          Date: 2017-05-01T06:06:11Z

          removed redundant logger declaration


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user sdutry opened a pull request: https://github.com/apache/struts/pull/136 WW-4793 only add JBossFileManager when supported You can merge this pull request into a Git repository by running: $ git pull https://github.com/sdutry/struts WW-4793 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/struts/pull/136.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #136 commit f7beeacf400ec6bdb27a7d67deb3b55dcc0f5dac Author: Stefaan Dutry <stefaan.dutry@gmail.com> Date: 2017-05-01T05:54:19Z added utility innerclass for checking support commit 4be974a482a2894b677ee4d2292ec778dfcd429e Author: Stefaan Dutry <stefaan.dutry@gmail.com> Date: 2017-05-01T06:00:54Z only add JBossFileManager when it's supported commit b140218cf2c7c634a6c42cd859d6d3b81cb5c290 Author: Stefaan Dutry <stefaan.dutry@gmail.com> Date: 2017-05-01T06:06:11Z removed redundant logger declaration
          Hide
          sdutry Stefaan Dutry added a comment -

          Pull request with possible solution created.

          Feel free to comment, suggest improvements or provide reasons why this is a bad idea.

          Show
          sdutry Stefaan Dutry added a comment - Pull request with possible solution created. Feel free to comment, suggest improvements or provide reasons why this is a bad idea.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user aleksandr-m commented on a diff in the pull request:

          https://github.com/apache/struts/pull/136#discussion_r114149826

          — Diff: core/src/main/java/org/apache/struts2/util/fs/JBossFileManager.java —
          @@ -210,4 +187,33 @@ private void addIfAbsent(List<URL> urls, URL fileUrl) {
          }
          }

          + public static class JBossFileManagerSupportUtil {
          — End diff –

          Why inner class? Why not just static methods in `JBossFileManager`?

          Show
          githubbot ASF GitHub Bot added a comment - Github user aleksandr-m commented on a diff in the pull request: https://github.com/apache/struts/pull/136#discussion_r114149826 — Diff: core/src/main/java/org/apache/struts2/util/fs/JBossFileManager.java — @@ -210,4 +187,33 @@ private void addIfAbsent(List<URL> urls, URL fileUrl) { } } + public static class JBossFileManagerSupportUtil { — End diff – Why inner class? Why not just static methods in `JBossFileManager`?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user sdutry commented on a diff in the pull request:

          https://github.com/apache/struts/pull/136#discussion_r114154579

          — Diff: core/src/main/java/org/apache/struts2/util/fs/JBossFileManager.java —
          @@ -210,4 +187,33 @@ private void addIfAbsent(List<URL> urls, URL fileUrl) {
          }
          }

          + public static class JBossFileManagerSupportUtil {
          — End diff –

          @aleksandr-m
          Good point, let me change that.

          Show
          githubbot ASF GitHub Bot added a comment - Github user sdutry commented on a diff in the pull request: https://github.com/apache/struts/pull/136#discussion_r114154579 — Diff: core/src/main/java/org/apache/struts2/util/fs/JBossFileManager.java — @@ -210,4 +187,33 @@ private void addIfAbsent(List<URL> urls, URL fileUrl) { } } + public static class JBossFileManagerSupportUtil { — End diff – @aleksandr-m Good point, let me change that.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user sdutry commented on a diff in the pull request:

          https://github.com/apache/struts/pull/136#discussion_r114157280

          — Diff: core/src/main/java/org/apache/struts2/util/fs/JBossFileManager.java —
          @@ -210,4 +187,33 @@ private void addIfAbsent(List<URL> urls, URL fileUrl) {
          }
          }

          + public static class JBossFileManagerSupportUtil {
          — End diff –

          @aleksandr-m
          Changed to static methods.
          (don't know why i didn't just do that from the start)

          Show
          githubbot ASF GitHub Bot added a comment - Github user sdutry commented on a diff in the pull request: https://github.com/apache/struts/pull/136#discussion_r114157280 — Diff: core/src/main/java/org/apache/struts2/util/fs/JBossFileManager.java — @@ -210,4 +187,33 @@ private void addIfAbsent(List<URL> urls, URL fileUrl) { } } + public static class JBossFileManagerSupportUtil { — End diff – @aleksandr-m Changed to static methods. (don't know why i didn't just do that from the start)
          Hide
          lukaszlenart Lukasz Lenart added a comment -

          I'm not sure what's the problem here, this line here

          configurationManager.addContainerProvider(new FileManagerProvider(JBossFileManager.class, "jboss"));
          

          only registers a new FileManager implementation under the name jboss, it's the same as with any other bean we have in struts-default.xml - it must be here (in Dispatcher) as the FileManager is used in a very early state of booting the framework.

          If you check usage of the FileManagerFactory#getFileManager() method you will notice it's used 6 times (7 to be correct but AnnotationActionValidatorManager is used by default instead of DefaultActionValidatorManager), so 12 calls is ok, 6 times when booting, and 6 times when reloading the framework after the initial boot.

          And that static method does exactly what the FileManager's support method does but right now you have introduced a hard dependency between Dispatcher and JBossFileManager using the static method - I'm not a fan of such things.

          The best option I see here is to remove the line configurationManager.addContainerProvider(new FileManagerProvider(JBossFileManager.class, "jboss")); and put a note into docs about how to configure Struts 2 when running on JBoss.

          https://struts.apache.org/docs/webxml.html#web.xml-CustomFileManagerandFileManagerFactoryimplementations

          Show
          lukaszlenart Lukasz Lenart added a comment - I'm not sure what's the problem here, this line here configurationManager.addContainerProvider( new FileManagerProvider(JBossFileManager.class, "jboss" )); only registers a new FileManager implementation under the name jboss , it's the same as with any other bean we have in struts-default.xml - it must be here (in Dispatcher ) as the FileManager is used in a very early state of booting the framework. If you check usage of the FileManagerFactory#getFileManager() method you will notice it's used 6 times (7 to be correct but AnnotationActionValidatorManager is used by default instead of DefaultActionValidatorManager ), so 12 calls is ok, 6 times when booting, and 6 times when reloading the framework after the initial boot. And that static method does exactly what the FileManager 's support method does but right now you have introduced a hard dependency between Dispatcher and JBossFileManager using the static method - I'm not a fan of such things. The best option I see here is to remove the line configurationManager.addContainerProvider(new FileManagerProvider(JBossFileManager.class, "jboss")); and put a note into docs about how to configure Struts 2 when running on JBoss. https://struts.apache.org/docs/webxml.html#web.xml-CustomFileManagerandFileManagerFactoryimplementations
          Hide
          lukaszlenart Lukasz Lenart added a comment -

          I have marked this issue to be resolved with 2.5.x but when you agree with my comment above, this should be moved to 2.6

          Show
          lukaszlenart Lukasz Lenart added a comment - I have marked this issue to be resolved with 2.5.x but when you agree with my comment above, this should be moved to 2.6
          Hide
          sdutry Stefaan Dutry added a comment -

          Lukasz Lenart

          If you check usage of the FileManagerFactory#getFileManager() method you will notice it's used 6 times (7 to be correct but AnnotationActionValidatorManager is used by default instead of DefaultActionValidatorManager), so 12 calls is ok, 6 times when booting, and 6 times when reloading the framework after the initial boot.

          It's true that this is only done at the moment of booting or reloading the framework, and therefore will never cause a performance issue. My thought was that this was a redundant thing to keep checking when you know it's not going to change after the first check, and in that way, even adding it to be checked every time. (technicaly support might be added at runtime using reflection on the classloader)

          And that static method does exactly what the FileManager's support method does but right now you have introduced a hard dependency between Dispatcher and JBossFileManager using the static method - I'm not a fan of such things.

          I'm afraid i don't fully understand what you mean here. How does adding a static method to the class that's already been referenced, and as far as i can tell, would always get instantiated, make this dependency harder? Please elaborate so i can take this into account in the future.

          The best option I see here is to remove the line configurationManager.addContainerProvider(new FileManagerProvider(JBossFileManager.class, "jboss")); and put a note into docs about how to configure Struts 2 when running on JBoss.

          Does this mean that the code wouldn't run on JBoss if the JBossFileManager isn't present in the configuration? If that's the case, i'd rather keep it as it is than remove the class. (maybe adding a minor configuration optimization to the docs for when not running on JBoss then)

          Thx for the feedback.

          Show
          sdutry Stefaan Dutry added a comment - Lukasz Lenart If you check usage of the FileManagerFactory#getFileManager() method you will notice it's used 6 times (7 to be correct but AnnotationActionValidatorManager is used by default instead of DefaultActionValidatorManager), so 12 calls is ok, 6 times when booting, and 6 times when reloading the framework after the initial boot. It's true that this is only done at the moment of booting or reloading the framework, and therefore will never cause a performance issue. My thought was that this was a redundant thing to keep checking when you know it's not going to change after the first check, and in that way, even adding it to be checked every time. (technicaly support might be added at runtime using reflection on the classloader) And that static method does exactly what the FileManager's support method does but right now you have introduced a hard dependency between Dispatcher and JBossFileManager using the static method - I'm not a fan of such things. I'm afraid i don't fully understand what you mean here. How does adding a static method to the class that's already been referenced, and as far as i can tell, would always get instantiated, make this dependency harder? Please elaborate so i can take this into account in the future. The best option I see here is to remove the line configurationManager.addContainerProvider(new FileManagerProvider(JBossFileManager.class, "jboss")); and put a note into docs about how to configure Struts 2 when running on JBoss. Does this mean that the code wouldn't run on JBoss if the JBossFileManager isn't present in the configuration? If that's the case, i'd rather keep it as it is than remove the class. (maybe adding a minor configuration optimization to the docs for when not running on JBoss then) Thx for the feedback.
          Hide
          lukaszlenart Lukasz Lenart added a comment -

          The checking is performed that way to avoid problems with synchronisation on threads but maybe this isn't an issue and the DefaultFileManagerFactory should store selected FileManager and re-use it instead of re-examining all the implementations.

          re: static method
          I don't like them, you cannot control when someone will use it and it also breaks separation of concerns and so on ...

          re: jboss
          nope, it will work only reloading of configs/properties in devMode won't work

          Show
          lukaszlenart Lukasz Lenart added a comment - The checking is performed that way to avoid problems with synchronisation on threads but maybe this isn't an issue and the DefaultFileManagerFactory should store selected FileManager and re-use it instead of re-examining all the implementations. re: static method I don't like them, you cannot control when someone will use it and it also breaks separation of concerns and so on ... re: jboss nope, it will work only reloading of configs/properties in devMode won't work
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user sdutry commented on the issue:

          https://github.com/apache/struts/pull/136

          Given the input on the issue, this pull request should not be merged.

          Show
          githubbot ASF GitHub Bot added a comment - Github user sdutry commented on the issue: https://github.com/apache/struts/pull/136 Given the input on the issue, this pull request should not be merged.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user sdutry closed the pull request at:

          https://github.com/apache/struts/pull/136

          Show
          githubbot ASF GitHub Bot added a comment - Github user sdutry closed the pull request at: https://github.com/apache/struts/pull/136
          Hide
          lukaszlenart Lukasz Lenart added a comment -

          tbh I still do not like the existing solution, if you have an idea how to improve that I will be glad to help you

          Show
          lukaszlenart Lukasz Lenart added a comment - tbh I still do not like the existing solution, if you have an idea how to improve that I will be glad to help you
          Hide
          sdutry Stefaan Dutry added a comment -

          Lukasz Lenart
          I'll see if i can think of something, but I'll need some more time to go through all the relevant code first.
          (Could take a couple of days before i find the time for it)

          Show
          sdutry Stefaan Dutry added a comment - Lukasz Lenart I'll see if i can think of something, but I'll need some more time to go through all the relevant code first. (Could take a couple of days before i find the time for it)
          Hide
          lukaszlenart Lukasz Lenart added a comment -

          Cool, take your time

          I thought about throwing away that whole FileManager thing as it's only needed in development phase, and instead develop a plugin that can be used during development which will have reloadable implementations of the beans. Another idea is to use commons-io or something to not bother with supporting your own FileManager. My two cents

          Show
          lukaszlenart Lukasz Lenart added a comment - Cool, take your time I thought about throwing away that whole FileManager thing as it's only needed in development phase, and instead develop a plugin that can be used during development which will have reloadable implementations of the beans. Another idea is to use commons-io or something to not bother with supporting your own FileManager . My two cents
          Hide
          sdutry Stefaan Dutry added a comment - - edited

          Lukasz Lenart

          I thought about throwing away that whole FileManager thing as it's only needed in development phase, and instead develop a plugin that can be used during development which will have reloadable implementations of the beans. Another idea is to use commons-io or something to not bother with supporting your own FileManager. My two cents

          • How do you mean: It's only needed in development phase
            Does that mean it's only used for reloading configurations and the application doesn't need it for initial startup and configuration loading?
          • for your other suggestions, if you could point me in the right direction, i wouldn't mind taking a look into them, however i think that's going to be a large modification.
            • a plugin for reloadable bean implementations
            • using commons-io

          For a small modifications i was thinking about one of the following:

          • adding a Field Boolean to JBossFileManager so it only checks once:
            • code change
              ...
              public class JBossFileManager extends DefaultFileManager {
                  ...
                  private Boolean supports;
                  ...
                  @Override
                  public boolean support() {
                      if (supports == null) { 
                          supports= isJBoss7() || isJBoss5();
              
                          if (supports) {
                              LOG.debug("JBoss server detected, Struts 2 will use [{}] to support file system operations!", JBossFileManager.class.getSimpleName());
                          }
                      }
              
                      return supports;
                  }
                 ...
              
            • upside: class lookups only happen once
            • downside: unboxing of boolean each check
          • as sugested, storing the FileManager in the DefaultFileManager
            ...
            public class DefaultFileManagerFactory implements FileManagerFactory {
                ...
                private FileManager fileManager
                ...
                public FileManager getFileManager() {
                    if (fileManager == null) {
                        fileManager = lookupFileManager();
            
                        if (fileManager != null) {
                            LOG.debug("Using FileManager implementation [{}]", fileManager.getClass().getSimpleName());
                        } else {
                            fileManager = systemFileManager;
                            LOG.debug("Using default implementation of FileManager provided under name [system]: {}", systemFileManager.getClass().getSimpleName());
                        }
            
                        fileManager.setReloadingConfigs(reloadingConfigs);
                    }
            
            
                    return fileManager;
                }
                ...
            

          I'd like to know your thoughts on this. (and the things i'm missing, especially concerning multithreading, as i'm not an expert there)

          Show
          sdutry Stefaan Dutry added a comment - - edited Lukasz Lenart I thought about throwing away that whole FileManager thing as it's only needed in development phase, and instead develop a plugin that can be used during development which will have reloadable implementations of the beans. Another idea is to use commons-io or something to not bother with supporting your own FileManager. My two cents How do you mean: It's only needed in development phase Does that mean it's only used for reloading configurations and the application doesn't need it for initial startup and configuration loading? for your other suggestions, if you could point me in the right direction, i wouldn't mind taking a look into them, however i think that's going to be a large modification. a plugin for reloadable bean implementations using commons-io For a small modifications i was thinking about one of the following: adding a Field Boolean to JBossFileManager so it only checks once: code change ... public class JBossFileManager extends DefaultFileManager { ... private Boolean supports; ... @Override public boolean support() { if (supports == null ) { supports= isJBoss7() || isJBoss5(); if (supports) { LOG.debug( "JBoss server detected, Struts 2 will use [{}] to support file system operations!" , JBossFileManager.class.getSimpleName()); } } return supports; } ... upside: class lookups only happen once downside: unboxing of boolean each check as sugested, storing the FileManager in the DefaultFileManager ... public class DefaultFileManagerFactory implements FileManagerFactory { ... private FileManager fileManager ... public FileManager getFileManager() { if (fileManager == null ) { fileManager = lookupFileManager(); if (fileManager != null ) { LOG.debug( "Using FileManager implementation [{}]" , fileManager.getClass().getSimpleName()); } else { fileManager = systemFileManager; LOG.debug( "Using default implementation of FileManager provided under name [system]: {}" , systemFileManager.getClass().getSimpleName()); } fileManager.setReloadingConfigs(reloadingConfigs); } return fileManager; } ... I'd like to know your thoughts on this. (and the things i'm missing, especially concerning multithreading, as i'm not an expert there)
          Hide
          lukaszlenart Lukasz Lenart added a comment -

          I think the second option with storing the selected FM in the FileManagerFactory is the best and the simplest.

          re: plugin
          Yes I know it's a huge task, maybe I should put this on my radar for 2.6

          Show
          lukaszlenart Lukasz Lenart added a comment - I think the second option with storing the selected FM in the FileManagerFactory is the best and the simplest. re: plugin Yes I know it's a huge task, maybe I should put this on my radar for 2.6
          Hide
          lukaszlenart Lukasz Lenart added a comment -

          Stefaan Dutry I will take over this task.

          Show
          lukaszlenart Lukasz Lenart added a comment - Stefaan Dutry I will take over this task.
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 05f3b7a9b9623b1e2c1378fa37c55b1d64bad20d in struts's branch refs/heads/master from Lukasz Lenart
          [ https://git-wip-us.apache.org/repos/asf?p=struts.git;h=05f3b7a ]

          WW-4793 Reduces overhead with looking for a FileManager

          Show
          jira-bot ASF subversion and git services added a comment - Commit 05f3b7a9b9623b1e2c1378fa37c55b1d64bad20d in struts's branch refs/heads/master from Lukasz Lenart [ https://git-wip-us.apache.org/repos/asf?p=struts.git;h=05f3b7a ] WW-4793 Reduces overhead with looking for a FileManager
          Hide
          sdutry Stefaan Dutry added a comment -

          Lukasz Lenart
          Ok, i didn't find the time to look further into this lately.
          (I briefly tried storing the selected FM in the FileManagerFactory but it made tests fail.)

          Show
          sdutry Stefaan Dutry added a comment - Lukasz Lenart Ok, i didn't find the time to look further into this lately. (I briefly tried storing the selected FM in the FileManagerFactory but it made tests fail.)
          Hide
          hudson Hudson added a comment -

          SUCCESS: Integrated in Jenkins build Struts-JDK7-master #638 (See https://builds.apache.org/job/Struts-JDK7-master/638/)
          WW-4793 Reduces overhead with looking for a FileManager (lukaszlenart: rev 05f3b7a9b9623b1e2c1378fa37c55b1d64bad20d)

          • (edit) core/src/main/java/com/opensymphony/xwork2/util/fs/DefaultFileManagerFactory.java
          • (edit) core/src/main/java/com/opensymphony/xwork2/FileManager.java
          Show
          hudson Hudson added a comment - SUCCESS: Integrated in Jenkins build Struts-JDK7-master #638 (See https://builds.apache.org/job/Struts-JDK7-master/638/ ) WW-4793 Reduces overhead with looking for a FileManager (lukaszlenart: rev 05f3b7a9b9623b1e2c1378fa37c55b1d64bad20d) (edit) core/src/main/java/com/opensymphony/xwork2/util/fs/DefaultFileManagerFactory.java (edit) core/src/main/java/com/opensymphony/xwork2/FileManager.java

            People

            • Assignee:
              lukaszlenart Lukasz Lenart
              Reporter:
              sdutry Stefaan Dutry
            • Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development