Bug 58790 - Issue in CheckDirty and its relation to ActionRouter
Summary: Issue in CheckDirty and its relation to ActionRouter
Status: RESOLVED FIXED
Alias: None
Product: JMeter
Classification: Unclassified
Component: Main (show other bugs)
Version: 2.13
Hardware: All All
: P2 minor (vote)
Target Milestone: ---
Assignee: JMeter issues mailing list
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-01-03 14:53 UTC by Philippe Mouawad
Modified: 2016-01-12 21:05 UTC (History)
1 user (show)



Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Philippe Mouawad 2016-01-03 14:53:29 UTC
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()
Comment 1 Sebb 2016-01-06 12:13:55 UTC
(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()
Comment 2 Philippe Mouawad 2016-01-06 12:21:33 UTC
(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()
Comment 3 Sebb 2016-01-06 14:00:14 UTC
(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.
Comment 4 Sebb 2016-01-06 14:01:25 UTC
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.
Comment 5 Philippe Mouawad 2016-01-06 20:46:29 UTC
(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.
Comment 6 Sebb 2016-01-07 02:55:20 UTC
(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.
Comment 7 Sebb 2016-01-07 06:33:43 UTC
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