Skip to content
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

StackOverflowError with ModuleController in Non-GUI mode if its name is the same as the target node #3180

Open
asfimport opened this issue Aug 7, 2013 · 10 comments

Comments

@asfimport
Copy link
Collaborator

christopher.roscoe (Bug 55375):
See attached a simple jmx file to reproduce the stackoverflow. There is no problem when i start the test in GUI mode.

In Non-GUI i have the following output.

An error occurred: null
errorlevel=1
Drücken Sie eine beliebige Taste . . .

Here is how i start the test in Non-GUI mode.

jmeter -n -t d:\development\workspaces\2.11.3\apache-jmeter-latest\bin\temp\stackoverflow\stackoverflow.jmx -l d:\development\workspaces\2.11.3\apache-jmeter-latest\bin\temp\stackoverflow\stackoverflow.jtl -j d:\development\workspaces\2.11.3\apache-jmeter-latest\bin\temp\stackoverflow\stackoverflow.log

In the log (also attached) the last entry is the stacktrace of the java.lang.StackOverflowError.

2013/08/07 10:04:45 FATAL - jmeter.JMeter: An error occurred: java.lang.StackOverflowError
at java.util.HashMap.containsKey(HashMap.java:335)
at org.apache.jorphan.collections.ListedHashTree.add(ListedHashTree.java:163)
at org.apache.jmeter.control.ModuleController.getReplacementSubTree(ModuleController.java:170)
at org.apache.jmeter.JMeter.convertSubTree(JMeter.java:883)
at org.apache.jmeter.JMeter.convertSubTree(JMeter.java:885)
at org.apache.jmeter.JMeter.convertSubTree(JMeter.java:885)
...

Created attachment stackoverflow.zip: The test to reproduce the error. The log file.

Severity: normal
OS: All

Duplicates:

Blocks:

@asfimport
Copy link
Collaborator Author

Sebb (migrated from Bugzilla):
Work-round is to rename the Module Controller so it has a different name from the target controller.

May perhaps be related to #2233?

@asfimport
Copy link
Collaborator Author

Sebb (migrated from Bugzilla):
Thanks for the original report and Test Case.

Turned out that the Module Controller (MC) was finding itself rather than the proper target node. Fixed by not allowing the target node to be a Module Controller.

URL: http://svn.apache.org/r1511236
Log:
StackOverflowError with ModuleController in Non-GUI mode if its name is the same as the target node
#3180

Added:
jmeter/trunk/bin/testfiles/Bug55375.csv (with props)
jmeter/trunk/bin/testfiles/Bug55375.jmx (with props)
jmeter/trunk/bin/testfiles/Bug55375.xml (with props)
Modified:
jmeter/trunk/build.xml
jmeter/trunk/src/components/org/apache/jmeter/control/ModuleController.java
jmeter/trunk/xdocs/changes.xml

@asfimport
Copy link
Collaborator Author

@pmouawad (migrated from Bugzilla):
Created attachment stackoverflow.jmx: Test Plan showing another way for issue to occur

stackoverflow.jmx
<?xml version="1.0" encoding="UTF-8"?>
<jmeterTestPlan version="1.2" properties="2.5" jmeter="2.10-SNAPSHOT.20130807">
  <hashTree>
    <TestPlan guiclass="TestPlanGui" testclass="TestPlan" testname="Test Plan" enabled="true">
      <stringProp name="TestPlan.comments"></stringProp>
      <boolProp name="TestPlan.functional_mode">false</boolProp>
      <boolProp name="TestPlan.serialize_threadgroups">true</boolProp>
      <elementProp name="TestPlan.user_defined_variables" elementType="Arguments" guiclass="ArgumentsPanel" testclass="Arguments" testname="User Defined Variables" enabled="true">
        <collectionProp name="Arguments.arguments"/>
      </elementProp>
      <stringProp name="TestPlan.user_define_classpath"></stringProp>
    </TestPlan>
    <hashTree>
      <ThreadGroup guiclass="ThreadGroupGui" testclass="ThreadGroup" testname="threadgroup" enabled="true">
        <stringProp name="ThreadGroup.on_sample_error">continue</stringProp>
        <elementProp name="ThreadGroup.main_controller" elementType="LoopController" guiclass="LoopControlPanel" testclass="LoopController" testname="Loop Controller" enabled="true">
          <boolProp name="LoopController.continue_forever">false</boolProp>
          <stringProp name="LoopController.loops">1</stringProp>
        </elementProp>
        <stringProp name="ThreadGroup.num_threads">1</stringProp>
        <stringProp name="ThreadGroup.ramp_time">0</stringProp>
        <longProp name="ThreadGroup.start_time">1344428409000</longProp>
        <longProp name="ThreadGroup.end_time">1344428409000</longProp>
        <boolProp name="ThreadGroup.scheduler">false</boolProp>
        <stringProp name="ThreadGroup.duration"></stringProp>
        <stringProp name="ThreadGroup.delay"></stringProp>
      </ThreadGroup>
      <hashTree>
        <GenericController guiclass="LogicControllerGui" testclass="GenericController" testname="reload" enabled="true"/>
        <hashTree>
          <BSFSampler guiclass="TestBeanGUI" testclass="BSFSampler" testname="reload" enabled="true">
            <stringProp name="scriptLanguage">groovy</stringProp>
            <stringProp name="parameters"></stringProp>
            <stringProp name="filename"></stringProp>
            <stringProp name="script">&quot;success&quot;</stringProp>
          </BSFSampler>
          <hashTree/>
        </hashTree>
        <TransactionController guiclass="TransactionControllerGui" testclass="TransactionController" testname="reload" enabled="true">
          <boolProp name="TransactionController.parent">false</boolProp>
        </TransactionController>
        <hashTree>
          <ModuleController guiclass="ModuleControllerGui" testclass="ModuleController" testname="reload" enabled="true">
            <collectionProp name="ModuleController.node_path">
              <stringProp name="-1227702913">WorkBench</stringProp>
              <stringProp name="764597751">Test Plan</stringProp>
              <stringProp name="1938366133">threadgroup</stringProp>
              <stringProp name="-934641255">reload</stringProp>
            </collectionProp>
          </ModuleController>
          <hashTree/>
        </hashTree>
      </hashTree>
      <ResultCollector guiclass="ViewResultsFullVisualizer" testclass="ResultCollector" testname="View Results Tree" enabled="true">
        <boolProp name="ResultCollector.error_logging">false</boolProp>
        <objProp>
          <name>saveConfig</name>
          <value class="SampleSaveConfiguration">
            <time>true</time>
            <latency>true</latency>
            <timestamp>true</timestamp>
            <success>true</success>
            <label>true</label>
            <code>true</code>
            <message>true</message>
            <threadName>true</threadName>
            <dataType>false</dataType>
            <encoding>false</encoding>
            <assertions>true</assertions>
            <subresults>false</subresults>
            <responseData>false</responseData>
            <samplerData>false</samplerData>
            <xml>false</xml>
            <fieldNames>true</fieldNames>
            <responseHeaders>false</responseHeaders>
            <requestHeaders>false</requestHeaders>
            <responseDataOnError>true</responseDataOnError>
            <saveAssertionResultsFailureMessage>true</saveAssertionResultsFailureMessage>
            <assertionsResultsToSave>0</assertionsResultsToSave>
            <bytes>true</bytes>
            <hostname>true</hostname>
            <threadCounts>true</threadCounts>
            <sampleCount>true</sampleCount>
          </value>
        </objProp>
        <stringProp name="filename"></stringProp>
      </ResultCollector>
      <hashTree/>
    </hashTree>
  </hashTree>
</jmeterTestPlan>

@asfimport
Copy link
Collaborator Author

Sebb (migrated from Bugzilla):
It's picking the wrong parent controller.

If you look in the drop-down list in the GUI, there are two instances of the target:

Test Plan > Thread Group > reload

Selecting the second one in the GUI results in a stack overflow error when running the GUI test.

It looks like the non-GUI code happens to choose the wrong target - correct name, wrong instance.

Note: the Test Plans that are created are identical, so the GUI test must be using additional information to identify the target controller.

Not sure if it makes sense to allow multiple MC targets with the same name.

If it does, then the JMX file (and drop-down list) will need to contain extra information to identify the instance. It may just be simpler to complain if the Test Plan contains multiple targets with the same name.

@asfimport
Copy link
Collaborator Author

@pmouawad (migrated from Bugzilla):
Created attachment BUG_55375.patch: Patch proposal to detect recursivity

BUG_55375.patch
Index: src/core/org/apache/jmeter/JMeter.java
===================================================================
--- src/core/org/apache/jmeter/JMeter.java	(revision 1550536)
+++ src/core/org/apache/jmeter/JMeter.java	(working copy)
@@ -82,6 +82,7 @@
 import org.apache.jmeter.util.BeanShellServer;
 import org.apache.jmeter.util.JMeterUtils;
 import org.apache.jorphan.collections.HashTree;
+import org.apache.jorphan.collections.HashTreeTraverser;
 import org.apache.jorphan.collections.SearchByClass;
 import org.apache.jorphan.gui.ComponentUtil;
 import org.apache.jorphan.logging.LoggingManager;
@@ -859,6 +860,7 @@
                         if (subTree != null) {
                             HashTree replacementTree = rc.getReplacementSubTree();
                             if (replacementTree != null) {
+                                checkForInfiniteRecursivity(replacementTree, rc);
                                 convertSubTree(replacementTree);
                                 tree.replace(item, rc);
                                 tree.set(rc, replacementTree);
@@ -884,6 +886,7 @@
                         if (subTree != null) {
                             HashTree replacementTree = rc.getReplacementSubTree();
                             if (replacementTree != null) {
+                                checkForInfiniteRecursivity(replacementTree, rc);
                                 convertSubTree(replacementTree);
                                 tree.replace(item, rc);
                                 tree.set(rc, replacementTree);
@@ -900,6 +903,72 @@
             }
         }
     }
+    
+    /**
+     * @since 2.10.1
+     */
+    private static final class InfiniteRecursivityDetector implements HashTreeTraverser {
+        boolean infiniteRecursivity;
+        private ReplaceableController replaceableController;
+
+        /**
+         * 
+         * @param nodeToFind
+         */
+        public InfiniteRecursivityDetector(ReplaceableController replaceableController) {
+            this.replaceableController = replaceableController;
+        }
+        
+        /**
+         * @return boolean
+         */
+        public boolean getInfiniteRecursivity() {
+            return infiniteRecursivity;
+        }
+        
+        @Override
+        public void subtractNode() {
+            // noop   
+        }
+        
+        @Override
+        public void processPath() {
+            // noop
+        }
+        
+        @Override
+        public void addNode(Object node, HashTree subTree) {
+            if(infiniteRecursivity== true) {
+                return;
+            }
+            if(node instanceof JMeterTreeNode) {
+                Object uo = ((JMeterTreeNode)node).getUserObject();
+                if(uo==replaceableController) {
+                    this.infiniteRecursivity = true;
+                }
+            }
+            if(node == replaceableController) {
+                this.infiniteRecursivity = true;
+            }            
+        }
+    }
+
+    /**
+     * Checks that replacementTree (built after replacement of ReplaceableController) does 
+     * not still contain ReplaceableController which would lead to infinite recursivity 
+     * see BUG 55375
+     * @param replacementTree {@link HashTreeTraverser}
+     * @param replaceableController {@link ReplaceableController}
+     */
+    private static void checkForInfiniteRecursivity(HashTree replacementTree,
+            final ReplaceableController replaceableController) {
+        InfiniteRecursivityDetector hashTreeTraverser = new InfiniteRecursivityDetector(replaceableController);
+        replacementTree.traverse(hashTreeTraverser);
+        if(hashTreeTraverser.getInfiniteRecursivity()) {
+            throw new IllegalStateException(
+                    "Recursivity detected in tree:"+((TestElement)replaceableController).getName());            
+        }
+    }
 
     /**
      * Ensures the {@link ReplaceableController} is loaded

@asfimport
Copy link
Collaborator Author

Sebb (migrated from Bugzilla):
The patch effectively converts the stack overflow into an illegal state exception.
The benefit is that the message reports the name offending test element (though this may not be unique).

However it does not solve the issue that the non-GUI code behaves differently from the GUI code.

Ideally any fix should address the different behaviour and prevent recursive calls.

If the MC refused to allow multiple controllers with the same name, that should solve the behaviour difference, but it could affect some test plans. Given that at present the non-GUI code does not have sufficient information to choose the correct controller in the case of duplicates, that change is probably acceptable.

The other issue is that the user may choose a target which is a parent of the MC, causing stack overflow. If controller names must be unique then this should be less likely to occur. Since such targets are useless, ideally they should be excluded from the list. This would need to be done after duplicate checking.

@asfimport
Copy link
Collaborator Author

@pmouawad (migrated from Bugzilla):
(In reply to Sebb from comment 6)

The patch effectively converts the stack overflow into an illegal state
exception.
The benefit is that the message reports the name offending test element
(though this may not be unique).
Shall I commit it ?

However it does not solve the issue that the non-GUI code behaves
differently from the GUI code.

In fact both GUI and Non GUI behave identically except that sometimes GUI picks the first element instead of picking the second one. But if you fix it then you get a StackOverflow.

Ideally any fix should address the different behaviour and prevent recursive
calls.

Fix prevents recursive calls.

If the MC refused to allow multiple controllers with the same name, that
should solve the behaviour difference, but it could affect some test plans.
Given that at present the non-GUI code does not have sufficient information
to choose the correct controller in the case of duplicates, that change is
probably acceptable.

Not sure

The other issue is that the user may choose a target which is a parent of
the MC, causing stack overflow. If controller names must be unique then this
should be less likely to occur. Since such targets are useless, ideally they
should be excluded from the list. This would need to be done after duplicate
checking.

Could be next step.

@asfimport
Copy link
Collaborator Author

@pmouawad (migrated from Bugzilla):
To be clear, I think the GUI may have in fact changed the saved reference value of Module Controller. So I suspect another bug in GUI mode which gets confused when there are duplicated.

@asfimport
Copy link
Collaborator Author

Sebb (migrated from Bugzilla):
I don't see any point in applying a temporary fix.

The GUI seems to remember the correct instance whilst it is active.
i.e. it will run which ever is selected from the list.

However, when the JMX is saved, only the name is stored.
So when the JMX is reloaded, it cannot possibly know which one was previously selected.
This is a fundamental problem for both GUI and non-GUI test runs.

One way to fix this is to insist that controller names are unique.
Another way would be to store the instance number.
This could either be done for every case, or only done where there are multiple matches, or the instance number could default to the first (i.e. only store it if not the first or only instance).

Requiring unique names would be more likely to break existing test plans, as the default names are not unique, so I now think it would be better to store an instance number. Making the instance default to 1 would reduce the number of test plans that needed to be updated. However it would make it more difficult to determine whether the choice was deliberate or accidental, unless the test plan version is also taken into account. So it might be better to insist that the instance number was always saved.

The question then arises - what should JMeter do if the user adds another controller with the same name after the MC entry has been selected?

@asfimport
Copy link
Collaborator Author

@pmouawad (migrated from Bugzilla):
(In reply to Sebb from comment 9)

I don't see any point in applying a temporary fix.
Ok, let's wait for a better implementation

The GUI seems to remember the correct instance whilst it is active.
i.e. it will run which ever is selected from the list.

However, when the JMX is saved, only the name is stored.
So when the JMX is reloaded, it cannot possibly know which one was
previously selected.
This is a fundamental problem for both GUI and non-GUI test runs.
Ok, that's what I was saying.

One way to fix this is to insist that controller names are unique.
Another way would be to store the instance number.

Maybe but could be tricky to develop and maintain.

This could either be done for every case, or only done where there are
multiple matches, or the instance number could default to the first (i.e.
only store it if not the first or only instance).

Requiring unique names would be more likely to break existing test plans, as
the default names are not unique, so I now think it would be better to store
an instance number. Making the instance default to 1 would reduce the number
of test plans that needed to be updated. However it would make it more
difficult to determine whether the choice was deliberate or accidental,
unless the test plan version is also taken into account. So it might be
better to insist that the instance number was always saved.

The question then arises - what should JMeter do if the user adds another
controller with the same name after the MC entry has been selected?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant