Jetty
  1. Jetty
  2. JETTY-1507

System properties are not getting cleared out between runs within an aggregator

    Details

    • Type: Bug Bug
    • Status: Resolved Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 7.6.1
    • Fix Version/s: 7.6.5, 8.1.5
    • Component/s: None
    • Labels:
      None
    • Environment:
    • Number of attachments :
      0

      Description

      If you have an aggregator with several modules each of which launches Jetty before the tests and if you've defined <systemProperties><systemProperty...></systemProperties, the next module to start up Jetty will end up using the values from defined for the first module, even if you've defined different values for this module.

        Activity

        Hide
        Greg Wilkins added a comment -

        Martin,

        Is this an issue with maven poms? Perhaps it is best to raise the issue with Maven?

        If you think it is a Jetty issue, can you give us more information and perhaps an example?

        cheers

        Show
        Greg Wilkins added a comment - Martin, Is this an issue with maven poms? Perhaps it is best to raise the issue with Maven? If you think it is a Jetty issue, can you give us more information and perhaps an example? cheers
        Hide
        Martin Todorov added a comment -

        Greg,

        This is an issue with the Jetty plugin, not with the properties defined in the pom. It's not a Maven problem per se. It's a problem with the plugin's handling of system properties.
        I removed all the systemProperties defined in the Jetty plugin. I then knocked up a simple Maven plugin, which sets system properties. With my plugin, the system properties are cleared between runs of the Jetty plugin in a multi-module aggregator which has more that one module running Jetty tests. (You can find my plugin here: https://github.com/carlspring/system-properties-maven-plugin ).

        Also, the plugin doesn't set system properties which have already been set. This is kind of a limitation.
        I worked around this problem by not using the <systemProperties> setting in the Jetty plugin and defining these properties in my plugin, but that should not be the way things work. In my opinion, the Jetty plugin should be able to override system properties and to make sure <systemProperties> are only defined for the scope of the execution.

        Show
        Martin Todorov added a comment - Greg, This is an issue with the Jetty plugin, not with the properties defined in the pom. It's not a Maven problem per se. It's a problem with the plugin's handling of system properties. I removed all the systemProperties defined in the Jetty plugin. I then knocked up a simple Maven plugin, which sets system properties. With my plugin, the system properties are cleared between runs of the Jetty plugin in a multi-module aggregator which has more that one module running Jetty tests. (You can find my plugin here: https://github.com/carlspring/system-properties-maven-plugin ). Also, the plugin doesn't set system properties which have already been set. This is kind of a limitation. I worked around this problem by not using the <systemProperties> setting in the Jetty plugin and defining these properties in my plugin, but that should not be the way things work. In my opinion, the Jetty plugin should be able to override system properties and to make sure <systemProperties> are only defined for the scope of the execution.
        Hide
        Jan Bartel added a comment -

        Martin,

        I agree this is jetty plugin behaviour. I remember implementing the setting this way purposefully, but I can't remember why. I agree that we should change it.

        Jan

        Show
        Jan Bartel added a comment - Martin, I agree this is jetty plugin behaviour. I remember implementing the setting this way purposefully, but I can't remember why. I agree that we should change it. Jan
        Hide
        Martin Todorov added a comment -

        Yeah, I actually had a look at the sources and I was struck by the fact that there is a specific if statement which checks if a system property has already been set and skips setting a new value. That's why I knocked up my workaround plugin. I tried looking around the code for comments explaining this behavior, but since I couldn't find any I decided to file a bug report as it's not exactly expected behavior.

        There could, for example, be a boolean configuration property which tells the Jetty plugin to not set new values for already defined system property. In my opinion, if such a property is introduced, it should actually default to false.

        Show
        Martin Todorov added a comment - Yeah, I actually had a look at the sources and I was struck by the fact that there is a specific if statement which checks if a system property has already been set and skips setting a new value. That's why I knocked up my workaround plugin. I tried looking around the code for comments explaining this behavior, but since I couldn't find any I decided to file a bug report as it's not exactly expected behavior. There could, for example, be a boolean configuration property which tells the Jetty plugin to not set new values for already defined system property. In my opinion, if such a property is introduced, it should actually default to false.
        Hide
        Jan Bartel added a comment -

        Ah, I remembered why I did it. You need to be able to have the command line override the System properties that might be set in the pom/programmatically.

        Jan

        Show
        Jan Bartel added a comment - Ah, I remembered why I did it. You need to be able to have the command line override the System properties that might be set in the pom/programmatically. Jan
        Hide
        Jan Bartel added a comment -

        The more I think about this, the less inclined I am to change the current behaviour. Consider that the SystemProperties are set as the plugin begins to execute. Any clear-down of them could not reasonably be done until jetty stops running. At that point, if we start changing the system properties, we may change the exit behaviour. For example, if a system property was set that affected the logging, perhaps resetting that would mean that an exception thrown on exit would not be logged (to the same logging framework used for the rest of the runtime).

        Instead of using <systemProperties> you could instead set your system properties in jetty.xml files that are executed with each new plugin invocation - they have normal jvm semantics and thus you can set them over and over again as suits your application.

        Jan

        Show
        Jan Bartel added a comment - The more I think about this, the less inclined I am to change the current behaviour. Consider that the SystemProperties are set as the plugin begins to execute. Any clear-down of them could not reasonably be done until jetty stops running. At that point, if we start changing the system properties, we may change the exit behaviour. For example, if a system property was set that affected the logging, perhaps resetting that would mean that an exception thrown on exit would not be logged (to the same logging framework used for the rest of the runtime). Instead of using <systemProperties> you could instead set your system properties in jetty.xml files that are executed with each new plugin invocation - they have normal jvm semantics and thus you can set them over and over again as suits your application. Jan
        Hide
        aaron pieper added a comment -

        Jan, when you said you can "set your system properties in jetty.xml files"... did you mean with a System.call invocation like what is described here (http://stackoverflow.com/a/4735302/255830) or is there a cleaner solution?

        Show
        aaron pieper added a comment - Jan, when you said you can "set your system properties in jetty.xml files"... did you mean with a System.call invocation like what is described here ( http://stackoverflow.com/a/4735302/255830 ) or is there a cleaner solution?
        Hide
        Jan Bartel added a comment -

        Yes, it would be something like that.

        Show
        Jan Bartel added a comment - Yes, it would be something like that.
        Hide
        Jan Bartel added a comment -

        Fixed.

        To have the system properties specified in the pom override those on the command line use <force>true</force>. Eg:

         <systemProperties>
            <force>true</force>
            <systemProperty>
               <name>fooprop</name>
               <value>222</value>
             </systemProperty>
          </systemProperties>
        
        Show
        Jan Bartel added a comment - Fixed. To have the system properties specified in the pom override those on the command line use <force>true</force>. Eg: <systemProperties> <force> true </force> <systemProperty> <name> fooprop </name> <value> 222 </value> </systemProperty> </systemProperties>
        Hide
        Martin Todorov added a comment -

        Jan, I never got to thank you for this, as your fix came a lot later than I needed it at the time and my workaround had worked then.
        I did however bump into this problem again today and, after Googling for a while, I ended up seeing my own question on Stackoverflow from two years ago (about which I'd totally forgotten) and that lead me back here to see there was a fix!

        So, many thanks once again!

        Show
        Martin Todorov added a comment - Jan, I never got to thank you for this, as your fix came a lot later than I needed it at the time and my workaround had worked then. I did however bump into this problem again today and, after Googling for a while, I ended up seeing my own question on Stackoverflow from two years ago (about which I'd totally forgotten) and that lead me back here to see there was a fix! So, many thanks once again!

          People

          • Assignee:
            Jan Bartel
            Reporter:
            Martin Todorov
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: