Currently there is an issue in ActionRouter#getInstance() It returns a partially initialized object so a fix would be to use a local object and affect it : public static ActionRouter getInstance() { if (router == null) { synchronized (LOCK) { if(router == null) { ActionRouter router = new ActionRouter(); router.populateCommandMap(); ActionRouter.router = router; } } } return router; } But due to CheckDirty using getInstance() in its constructor to register itself as a PreAction to ExitCommand, it leads to a StackoverflowError. Even without fixing this issue, it reveals an issue that could occur if a Constructor of an AbstractCommand subclass uses ActionRouter.getInstance()
(In reply to Philippe Mouawad from comment #0) > Currently there is an issue in ActionRouter#getInstance() > It returns a partially initialized object so a fix would be to use a local > object and affect it : > public static ActionRouter getInstance() { > if (router == null) { > synchronized (LOCK) { > if(router == null) { > ActionRouter router = new ActionRouter(); > router.populateCommandMap(); > ActionRouter.router = router; > } > } > } > return router; > } Alternatively one could use the IODH idiom. > > But due to CheckDirty using getInstance() in its constructor to register > itself as a PreAction to ExitCommand, it leads to a StackoverflowError. Not sure I follow this. > Even without fixing this issue, it reveals an issue that could occur if a > Constructor of an AbstractCommand subclass uses ActionRouter.getInstance()
(In reply to Sebb from comment #1) > (In reply to Philippe Mouawad from comment #0) > > Currently there is an issue in ActionRouter#getInstance() > > It returns a partially initialized object so a fix would be to use a local > > object and affect it : > > public static ActionRouter getInstance() { > > if (router == null) { > > synchronized (LOCK) { > > if(router == null) { > > ActionRouter router = new ActionRouter(); > > router.populateCommandMap(); > > ActionRouter.router = router; > > } > > } > > } > > return router; > > } > > Alternatively one could use the IODH idiom. > > > > > But due to CheckDirty using getInstance() in its constructor to register > > itself as a PreAction to ExitCommand, it leads to a StackoverflowError. > > Not sure I follow this. Try you will see > > > Even without fixing this issue, it reveals an issue that could occur if a > > Constructor of an AbstractCommand subclass uses ActionRouter.getInstance()
(In reply to Philippe Mouawad from comment #2) > > > But due to CheckDirty using getInstance() in its constructor to register > > > itself as a PreAction to ExitCommand, it leads to a StackoverflowError. > > > > Not sure I follow this. > Try you will see Try what exactly? JMeter works fine for me if I try to exit without saving an updated test plan.
Note: I agree that the code needs to be changed because there's a clear threading issue. However I have not seen any related failures.
(In reply to Sebb from comment #3) > (In reply to Philippe Mouawad from comment #2) > > > > But due to CheckDirty using getInstance() in its constructor to register > > > > itself as a PreAction to ExitCommand, it leads to a StackoverflowError. > > > > > > Not sure I follow this. > > Try you will see > > Try what exactly? > > JMeter works fine for me if I try to exit without saving an updated test > plan. Try the code that I wrote and run the junit tests.
(In reply to Philippe Mouawad from comment #5) > (In reply to Sebb from comment #3) > > (In reply to Philippe Mouawad from comment #2) > > > > > But due to CheckDirty using getInstance() in its constructor to register > > > > > itself as a PreAction to ExitCommand, it leads to a StackoverflowError. > > > > > > > > Not sure I follow this. > > > Try you will see > > > > Try what exactly? > > > > JMeter works fine for me if I try to exit without saving an updated test > > plan. > > Try the code that I wrote and run the junit tests. Sorry, I see now, you meant that the fix causes the Stack Overflow. IODH won't help here. A possible work-round is for ActionRouter to add CheckDirty as a PreAction Listener after it has finished populating the command map. i.e. we ban the use of getInstance from classes that implement Command. I don't like that. Another might be to get org.apache.jmeter.JMeter.startGui to run the populateCommandMap code instead of doing it in getInstance. AFAICT that is the first place where ActionRouter is created. This would mean making populateCommandMap public, but it could protect itself from repeated invocations by having a done flag (or seeing if there are any commands). A quick test shows this seems to work.
Fixed: URL: http://svn.apache.org/viewvc?rev=1723468&view=rev Log: Issue in CheckDirty and its relation to ActionRouter Bugzilla Id: 58790 Modified: jmeter/trunk/src/core/org/apache/jmeter/JMeter.java jmeter/trunk/src/core/org/apache/jmeter/gui/action/ActionRouter.java jmeter/trunk/xdocs/changes.xml
This issue has been migrated to GitHub: https://github.com/apache/jmeter/issues/3751