JBehave
  1. JBehave
  2. JBEHAVE-492

CandidateSteps instances should be created by StoryRunner context allowing for multi-threaded stateful steps logic

    Details

    • Type: New Feature New Feature
    • Status: Resolved Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 3.x
    • Fix Version/s: 3.4
    • Component/s: Core
    • Labels:
      None
    • Number of attachments :
      0

      Description

      In multi-threaded mode, the steps instances need to be instantiated per thread.

      A way to do this is to pass the InjectableStepsFactory to the StoryRunner run context and let the context instantiate the candidate steps per thread.

        Activity

        Mauro Talevi made changes -
        Field Original Value New Value
        Summary ConfigurableSteps.addSteps(..) should take classes not instances. ConfigurableEmbedder.addSteps(..) should take classes not instances.
        Issue Type Bug [ 1 ] New Feature [ 2 ]
        Fix Version/s 3.x [ 16979 ]
        Hide
        Paul Hammant added a comment -

        End-users. In the meantime, if you're trying to do the WebDriver PageFactory style of field initialization in the constructor to BasePage (refer http://code.google.com/p/selenium/wiki/PageFactory), change from this style:

        PageFactory.initElements(driverProvider.get(), this);

        .. to this style:

        try

        { PageFactory.initElements(driverProvider.get(), this); }

        catch (Exception e)

        { // this is OK, the first instance of each page is created by JBehave for the purposes of inspecting the methods of steps classes. The instance itself is not needed. Therefore other methods in this class are not called for an instance that has skipped the WebDriver PageFactory style field initialization. }
        Show
        Paul Hammant added a comment - End-users. In the meantime, if you're trying to do the WebDriver PageFactory style of field initialization in the constructor to BasePage (refer http://code.google.com/p/selenium/wiki/PageFactory ), change from this style: PageFactory.initElements(driverProvider.get(), this); .. to this style: try { PageFactory.initElements(driverProvider.get(), this); } catch (Exception e) { // this is OK, the first instance of each page is created by JBehave for the purposes of inspecting the methods of steps classes. The instance itself is not needed. Therefore other methods in this class are not called for an instance that has skipped the WebDriver PageFactory style field initialization. }
        Hide
        Brian Repko added a comment -

        Hunh? When using the DI/IoC container the steps classes are created with the container and we pass in instances. They are singletons for the most part and should work just fine in a multi-threaded environment but they aren't created by JBehave.

        The Spring-Security example shows this with spring but I think the general trader examples do this as well. I'm pretty sure you don't want JBehave to get into the fun intricacies of object creation.

        Show
        Brian Repko added a comment - Hunh? When using the DI/IoC container the steps classes are created with the container and we pass in instances. They are singletons for the most part and should work just fine in a multi-threaded environment but they aren't created by JBehave. The Spring-Security example shows this with spring but I think the general trader examples do this as well. I'm pretty sure you don't want JBehave to get into the fun intricacies of object creation.
        Hide
        Mauro Talevi added a comment -

        Paul, why do you say the instance of a steps class is never used? How are the methods executed?

        And as Brian was rightfully noting, how would DI work?

        Isn't your usecase too influenced by the use of Groovy to drive WebDriver?

        Show
        Mauro Talevi added a comment - Paul, why do you say the instance of a steps class is never used? How are the methods executed? And as Brian was rightfully noting, how would DI work? Isn't your usecase too influenced by the use of Groovy to drive WebDriver?
        Hide
        Paul Hammant added a comment -

        Added word *initial* 3x to decrease confusion about problem/solution

        Show
        Paul Hammant added a comment - Added word * initial * 3x to decrease confusion about problem/solution
        Paul Hammant made changes -
        Description public void addSteps(List<Class> steps) {
        }

        ... would be better.

        Why? There's an instance that's made during the setup of a suite (refer EtsyStories). If you are running in multi-threaded mode, that instance is **never** used during the running of scenarios.

        In multi-threaded mode, there's a constructor for BasePage that takes a WebDriverProvider. When it is passed in during the primordial instantiation phase, no WebDriver has been initialized. We don't want to do that of course because (a) we know this instance is going to be garbage collected and never used, and (b) on SauceLabs at least it would result in browser opening in the cloud then closing (or maybe not closing, but never used; I'm not sure).

        JBehave does not actually need the instance. I believe this is provable. Instead it could perfectly well use the class definition without the instance ref somehow.

        Proposal: Long overdue, I think, but could for 4.x we shift to using class-defs. Either rework CandidateSteps to hold a Class rather than an instance. Or further do a away (move the logic elsewhere) with CandidateSteps, and use the POJO Step's class-def directly.
        public void addSteps(List<Class> steps) {
        }

        ... would be better.

        Why? There's an instance that's made during the setup of a suite (refer EtsyStories). If you are running in multi-threaded mode, that **initial** instance is **never** used during the running of scenarios.

        In multi-threaded mode, there's a constructor for BasePage that takes a WebDriverProvider. When it is passed in during the primordial instantiation phase, no WebDriver has been initialized. We don't want to do that of course because (a) we know this **initial** instance is going to be garbage collected and never used, and (b) on SauceLabs at least it would result in browser opening in the cloud then closing (or maybe not closing, but never used; I'm not sure).

        JBehave does not actually need the **initial** instance. I believe this is provable. Instead it could perfectly well use the class definition without the instance ref somehow.

        Proposal: Long overdue, I think, but could for 4.x we shift to using class-defs. Either rework CandidateSteps to hold a Class rather than an instance. Or further do a away (move the logic elsewhere) with CandidateSteps, and use the POJO Step's class-def directly.
        Hide
        Mauro Talevi added a comment -

        Sorry, still no wiser about this use case What do you mean by "initial instance"?

        Show
        Mauro Talevi added a comment - Sorry, still no wiser about this use case What do you mean by "initial instance"?
        Hide
        Brian Repko added a comment -

        Is this more related to multi-threading? Perhaps the design for multi-threaded execution needs to be worked out more?

        Show
        Brian Repko added a comment - Is this more related to multi-threading? Perhaps the design for multi-threaded execution needs to be worked out more?
        Hide
        Paul Hammant added a comment - - edited

        So the Etsy-Stories module in 'tutorial' is using a ThreadLocal caching mechanism. This is to allow the multi-threaded running of tests on stacks like SauceLabs.com. As any of the page objects or steps classes may maintain state, they need to be separate instances.

        JBehave (as correctly suggested by Brian) does not instantiate page objects, steps classes or other dependent components..... PicoContainer does (for the Etsy example).

        Refer https://github.com/jbehave/jbehave-tutorial/blob/master/etsy-stories/src/main/java/org/jbehave/tutorials/etsy/EtsyDotComStories.java. Search in page for ThreadCaching, see that it is used in a container's instantiation (all comps in that container will be 'new' per thread). the makeContainer() invocation makes a child container with the same spec (ThreadCaching). Thus the comps registered at that level are newed up per thread too.

        Thus, and I am 100% sure about this. Any instances made by PicoStepsFactory (line 126 presently) are not used by actual step invocation, nor those steps instances invocation into page objects. At least when in multithreaded mode. In regular single-threaded mode, because we coded a class 'NonThreadingExecutorService' the (serial) execution thread will be the same as the thread that passed through the constructor of EtsyStories.

        So. In my use-case, these are not singletons because of the multithreaded needs because of the concurrent leasing of browsers from SauceLabs. In my use-case I'm 100% that EtsyDotComSteps, housekeeping.EmptyCartIfNotAlready and the seven Groovy classes in the pages package are instantiated needlessly at during the construction phase of EtsyStories.

        Show
        Paul Hammant added a comment - - edited So the Etsy-Stories module in 'tutorial' is using a ThreadLocal caching mechanism. This is to allow the multi-threaded running of tests on stacks like SauceLabs.com. As any of the page objects or steps classes may maintain state, they need to be separate instances. JBehave (as correctly suggested by Brian) does not instantiate page objects, steps classes or other dependent components..... PicoContainer does (for the Etsy example). Refer https://github.com/jbehave/jbehave-tutorial/blob/master/etsy-stories/src/main/java/org/jbehave/tutorials/etsy/EtsyDotComStories.java . Search in page for ThreadCaching, see that it is used in a container's instantiation (all comps in that container will be 'new' per thread). the makeContainer() invocation makes a child container with the same spec (ThreadCaching). Thus the comps registered at that level are newed up per thread too. Thus, and I am 100% sure about this. Any instances made by PicoStepsFactory (line 126 presently) are not used by actual step invocation, nor those steps instances invocation into page objects. At least when in multithreaded mode. In regular single-threaded mode, because we coded a class 'NonThreadingExecutorService' the (serial) execution thread will be the same as the thread that passed through the constructor of EtsyStories. So. In my use-case, these are not singletons because of the multithreaded needs because of the concurrent leasing of browsers from SauceLabs. In my use-case I'm 100% that EtsyDotComSteps, housekeeping.EmptyCartIfNotAlready and the seven Groovy classes in the pages package are instantiated needlessly at during the construction phase of EtsyStories.
        Hide
        Paul Hammant added a comment - - edited

        If you don't believe me, add a break-point to, say, CartContent's constructor and run the three tests on SauceLabs.

        Show
        Paul Hammant added a comment - - edited If you don't believe me, add a break-point to, say, CartContent's constructor and run the three tests on SauceLabs.
        Hide
        Mauro Talevi added a comment -

        So when are the steps classes instantiated when running in SauceLabs? Is this specific to the use of Groovy (effectively you're using a default constructor and GrooBe to autowire your pages) or generic for all Java classes?

        Show
        Mauro Talevi added a comment - So when are the steps classes instantiated when running in SauceLabs? Is this specific to the use of Groovy (effectively you're using a default constructor and GrooBe to autowire your pages) or generic for all Java classes?
        Hide
        Paul Hammant added a comment -

        Here's the initial stack trace (EtsyStories constructor): http://pastebin.com/RXNFR157

        Show
        Paul Hammant added a comment - Here's the initial stack trace (EtsyStories constructor): http://pastebin.com/RXNFR157
        Hide
        Brian Repko added a comment -

        Got it - thanks, Paul.

        ThreadCaching is interesting - in Spring, bean scopes are singleton, prototype (new bean for each get call) and a bunch related to web (request/session/global-session-for-portlets) but nothing like once-per-thread. Problem I see with that is that its a global policy on all objects in the container rather than per-bean.

        And obviously the kicker is the instance variables during multi-threaded execution. One could make the steps classes singletons that then reach out to threadlocals (or threadlocal wrappers) to get data that would be instance variables. But then the global nature of the ThreadCaching will hit you (some objects are thread-specific and some aren't).

        So if you really need new steps classes (and others) per thread - then what I'm not sure about is how we get the candidate steps in the configuration to be thread-specific...I'd have to think about that design a bit more...

        Show
        Brian Repko added a comment - Got it - thanks, Paul. ThreadCaching is interesting - in Spring, bean scopes are singleton, prototype (new bean for each get call) and a bunch related to web (request/session/global-session-for-portlets) but nothing like once-per-thread. Problem I see with that is that its a global policy on all objects in the container rather than per-bean. And obviously the kicker is the instance variables during multi-threaded execution. One could make the steps classes singletons that then reach out to threadlocals (or threadlocal wrappers) to get data that would be instance variables. But then the global nature of the ThreadCaching will hit you (some objects are thread-specific and some aren't). So if you really need new steps classes (and others) per thread - then what I'm not sure about is how we get the candidate steps in the configuration to be thread-specific...I'd have to think about that design a bit more...
        Hide
        Brian Repko added a comment -

        Take that back - Spring 3.0 has thread scope per bean. I'm assuming you don't want to try that in Spring? (he laughs as he writes...)

        Show
        Brian Repko added a comment - Take that back - Spring 3.0 has thread scope per bean. I'm assuming you don't want to try that in Spring? (he laughs as he writes...)
        Hide
        Paul Hammant added a comment -

        In Pico-land you could register a component by a) instance to avoid the ThreadCaching stuff, b) have an even more primordial container that is plain 'Caching' (equiv to Spring's Singleton scope), or c) register something by type in the otherwise ThreadCaching container, but with an explicit Cached behavior (for that there's about three ways), but here is one way:

        steps1.as(CACHED).addComponent(new ClassName("housekeeping.EmptyCartIfNotAlready"));

        My problem right now is that I'm pinging back and forth from the case in work (that's busted on WebDriverProvider.get() in BasePage's ctor) and equivalent stuff in the Etsy tutorial which is not as sophisticated

        Show
        Paul Hammant added a comment - In Pico-land you could register a component by a) instance to avoid the ThreadCaching stuff, b) have an even more primordial container that is plain 'Caching' (equiv to Spring's Singleton scope), or c) register something by type in the otherwise ThreadCaching container, but with an explicit Cached behavior (for that there's about three ways), but here is one way: steps1.as(CACHED).addComponent(new ClassName("housekeeping.EmptyCartIfNotAlready")); My problem right now is that I'm pinging back and forth from the case in work (that's busted on WebDriverProvider.get() in BasePage's ctor) and equivalent stuff in the Etsy tutorial which is not as sophisticated
        Hide
        Paul Hammant added a comment -

        > So if you really need new steps classes (and others)
        > per thread - then what I'm not sure about is how we
        > get the candidate steps in the configuration to be
        > thread-specific...I'd have to think about that
        > design a bit more...

        My ideal is that the startup phase of JBehave deals with class-defs (kinda like DI containers do). Later when instances are needed for step execution, they are acquired from one of: Pico, Spring, Guice, Weld ... or a default non-DI JBehave container where everything was pre-instantiated.

        Show
        Paul Hammant added a comment - > So if you really need new steps classes (and others) > per thread - then what I'm not sure about is how we > get the candidate steps in the configuration to be > thread-specific...I'd have to think about that > design a bit more... My ideal is that the startup phase of JBehave deals with class-defs (kinda like DI containers do). Later when instances are needed for step execution, they are acquired from one of: Pico, Spring, Guice, Weld ... or a default non-DI JBehave container where everything was pre-instantiated.
        Hide
        Mauro Talevi added a comment -

        To do that, we'd loose the nice abstraction that we have wrt multiple DI containers. And not at all easy given the current design.

        Rather, it seems to me that we should focus on making the configuration thread-specific.

        Passing the classes instead of the instances is not necessarily the only way to achieve the stated goal.

        Show
        Mauro Talevi added a comment - To do that, we'd loose the nice abstraction that we have wrt multiple DI containers. And not at all easy given the current design. Rather, it seems to me that we should focus on making the configuration thread-specific. Passing the classes instead of the instances is not necessarily the only way to achieve the stated goal.
        Hide
        Paul Hammant added a comment -

        I'm still digging through code. I think things (around multi-threadedness) is worse that I thought.

        Show
        Paul Hammant added a comment - I'm still digging through code. I think things (around multi-threadedness) is worse that I thought.
        Hide
        Paul Hammant added a comment -

        You guys may be right - only the fist instance made by implementors of AbstractStepsFactory.stepsInstances() is used. Attempts to make separate instances available on ThreadLocal are ignored

        Show
        Paul Hammant added a comment - You guys may be right - only the fist instance made by implementors of AbstractStepsFactory.stepsInstances() is used. Attempts to make separate instances available on ThreadLocal are ignored
        Hide
        Paul Hammant added a comment -

        OK, here's the unfortunate way we'll have to do this I think :- https://github.com/jbehave/jbehave-tutorial/commit/7b642bc1d692edfd2dffe098c642e29cba856ab4

        The steps containers cannot attempt to do ThreadLocal because JBehave works with instances instantiated up front.

        However, injected deps (into pages and/or steps classes depending on the number of containers) can be ThreadLocal managed (ThreadCached for PicoContainer). This commit illustrates that.

        Unfortunately, this means the end-user needs to think about statelessness in at least steps classes.

        Show
        Paul Hammant added a comment - OK, here's the unfortunate way we'll have to do this I think :- https://github.com/jbehave/jbehave-tutorial/commit/7b642bc1d692edfd2dffe098c642e29cba856ab4 The steps containers cannot attempt to do ThreadLocal because JBehave works with instances instantiated up front. However, injected deps (into pages and/or steps classes depending on the number of containers) can be ThreadLocal managed (ThreadCached for PicoContainer). This commit illustrates that. Unfortunately, this means the end-user needs to think about statelessness in at least steps classes.
        Mauro Talevi made changes -
        Summary ConfigurableEmbedder.addSteps(..) should take classes not instances. CandidateSteps instances should be created by StoryRunner context allowing for multi-threaded stateful steps logic
        Mauro Talevi made changes -
        Assignee Mauro Talevi [ maurotalevi ]
        Fix Version/s 3.4 [ 17278 ]
        Fix Version/s 3.x [ 16979 ]
        Mauro Talevi made changes -
        Description public void addSteps(List<Class> steps) {
        }

        ... would be better.

        Why? There's an instance that's made during the setup of a suite (refer EtsyStories). If you are running in multi-threaded mode, that **initial** instance is **never** used during the running of scenarios.

        In multi-threaded mode, there's a constructor for BasePage that takes a WebDriverProvider. When it is passed in during the primordial instantiation phase, no WebDriver has been initialized. We don't want to do that of course because (a) we know this **initial** instance is going to be garbage collected and never used, and (b) on SauceLabs at least it would result in browser opening in the cloud then closing (or maybe not closing, but never used; I'm not sure).

        JBehave does not actually need the **initial** instance. I believe this is provable. Instead it could perfectly well use the class definition without the instance ref somehow.

        Proposal: Long overdue, I think, but could for 4.x we shift to using class-defs. Either rework CandidateSteps to hold a Class rather than an instance. Or further do a away (move the logic elsewhere) with CandidateSteps, and use the POJO Step's class-def directly.
        In multi-threaded mode, the steps instances need to be instantiated per thread.

        A way to do this is to pass the InjectableStepsFactory to the StoryRunner run context and let the context instantiate the candidate steps per thread.

        Mauro Talevi made changes -
        Status Open [ 1 ] In Progress [ 3 ]
        Mauro Talevi made changes -
        Status In Progress [ 3 ] Resolved [ 5 ]
        Resolution Fixed [ 1 ]

          People

          • Assignee:
            Mauro Talevi
            Reporter:
            Paul Hammant
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: