Tapestry 5
  1. Tapestry 5
  2. TAP5-128

Render phase short circuiting fails to abort the event when mixins are present on the component, resulting in an IllegalStateException when trying to store a subsequent result value

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 5.0.16
    • Component/s: None
    • Labels:
      None

      Description

      According to the docs on render phase short-circuiting for methods of the same phase:
      http://tapestry.apache.org/tapestry5/tapestry-core/guide/rendering.html

      "If a method returns a true or false value, this will short circuit processing.
      Other methods within the phase that would ordinarily be invoked will not be invoked."

      This is currently unimplemented. (Only other "inner" phases are short-circuited & not invoked.)

      Changes would primarily affect:
      org.apache.tapestry.internal.structure.ComponentCallback
      org.apache.tapestry.internal.structure.ComponentPageElementImpl

      In ComponentPageElementImpl.invoke(...), the loop:
      while (i.hasNext())
      callback.run(i.next());

      needs to implement the short circuiting, for example:
      while (i.hasNext())
      if (callback.run(i.next()))
      break;

      This change would cascade up to ComponentCallback and all its usage in ComponentPageElementImpl.

      Cheers,
      Nick.

      1. jira1662sample.tgz
        7 kB
        Robert Zeigler

        Activity

        Howard M. Lewis Ship made changes -
        Fix Version/s 5.0.16 [ 12313427 ]
        Status Reopened [ 4 ] Closed [ 6 ]
        Resolution Fixed [ 1 ]
        Hide
        Howard M. Lewis Ship added a comment -

        Thanks for the sample app; I updated it for 5.0.15 and verified the problem, then tried the fix. Had to modify the mixins to return false from beforeRenderBody(), but then everything worked as expected (the Any component omitted its body).

        Show
        Howard M. Lewis Ship added a comment - Thanks for the sample app; I updated it for 5.0.15 and verified the problem, then tried the fix. Had to modify the mixins to return false from beforeRenderBody(), but then everything worked as expected (the Any component omitted its body).
        Howard M. Lewis Ship made changes -
        Assignee Howard M. Lewis Ship [ hlship ]
        Summary Render phase short-circuiting is not implemented for methods of the same phase. Render phase short circuiting fails to abort the event when mixins are present on the component, resulting in an IllegalStateException when trying to store a subsequent result value
        Hide
        Howard M. Lewis Ship added a comment -

        I can see a problem in this code:

        private void invoke(boolean reverse, ComponentCallback callback)
        {
        try
        { // Optimization: In the most general case (just the one component, no mixins)
        // invoke the callback on the component and be done ... no iterators, no nothing.

        if (components == null)

        { callback.run(coreComponent); return; }

        Iterator<Component> i = reverse ? InternalUtils.reverseIterator(components) : components.iterator();

        while (i.hasNext()) callback.run(i.next());
        }
        catch (RuntimeException ex)

        { throw new TapestryException(ex.getMessage(), getLocation(), ex); }

        }

        Looks like we work our way through the full components list (mixins plus the component itself) even once the event is aborted (because a non-null/non-void value was returned).

        Show
        Howard M. Lewis Ship added a comment - I can see a problem in this code: private void invoke(boolean reverse, ComponentCallback callback) { try { // Optimization: In the most general case (just the one component, no mixins) // invoke the callback on the component and be done ... no iterators, no nothing. if (components == null) { callback.run(coreComponent); return; } Iterator<Component> i = reverse ? InternalUtils.reverseIterator(components) : components.iterator(); while (i.hasNext()) callback.run(i.next()); } catch (RuntimeException ex) { throw new TapestryException(ex.getMessage(), getLocation(), ex); } } Looks like we work our way through the full components list (mixins plus the component itself) even once the event is aborted (because a non-null/non-void value was returned).
        Howard M. Lewis Ship made changes -
        Project Tapestry [ 10573 ] Tapestry 5 [ 12310833 ]
        Key TAPESTRY-1662 TAP5-128
        Affects Version/s 5.0.5 [ 12312477 ]
        Component/s tapestry-core [ 12311285 ]
        Howard M. Lewis Ship made changes -
        Assignee Howard M. Lewis Ship [ hlship ]
        Hide
        Nick Westgate added a comment -

        Thanks a lot for the example Robert. I should have added one, but have just had zero free time for months now ...

        Cheers,
        Nick.

        Show
        Nick Westgate added a comment - Thanks a lot for the example Robert. I should have added one, but have just had zero free time for months now ... Cheers, Nick.
        Robert Zeigler made changes -
        Attachment jira1662sample.tgz [ 12381973 ]
        Hide
        Robert Zeigler added a comment -

        The file I attached contains a tapestry project, generated with the quickstart archetype, that illustrates the bug.
        In short:
        There are two mixins: MixinBefore and MixinBefore2.
        In Index.tml, an "any" component is given the mixins as: t:mixins="MixedBefore,MixedBefore2".
        So MixinBefore is called first, and then MixingBefore2.
        MixinBefore's beginRender method returns false; this should short-circuit rendering of the body of the "any" component, as diagramed in the component state diagram at:
        http://tapestry.apache.org/tapestry5/tapestry-core/guide/rendering.html
        It will short-circuit it, but according to the last sub-section of the same documentation:

        "Short Circuiting

        If a method returns a true or false value, this will short circuit processing. Other methods within the phase that would ordinarily be invoked will not be invoked.

        Most render phase methods should return void, to avoid unintentionally short circuiting other methods for the same phase."

        This suggests that the MixedBefore2's beginRender method should NOT be called.
        So, in MixedBefore2's beginRender method, an IllegalStateException is thrown.

        If you start up the app, and go to the page, you will see that the IllegalStateException is thrown.

        As Nick pointed out, either the documentation needs to be fixed, or the code needs to be fixed, but they are clearly at odds.

        Show
        Robert Zeigler added a comment - The file I attached contains a tapestry project, generated with the quickstart archetype, that illustrates the bug. In short: There are two mixins: MixinBefore and MixinBefore2. In Index.tml, an "any" component is given the mixins as: t:mixins="MixedBefore,MixedBefore2". So MixinBefore is called first, and then MixingBefore2. MixinBefore's beginRender method returns false; this should short-circuit rendering of the body of the "any" component, as diagramed in the component state diagram at: http://tapestry.apache.org/tapestry5/tapestry-core/guide/rendering.html It will short-circuit it, but according to the last sub-section of the same documentation: "Short Circuiting If a method returns a true or false value, this will short circuit processing. Other methods within the phase that would ordinarily be invoked will not be invoked. Most render phase methods should return void, to avoid unintentionally short circuiting other methods for the same phase." This suggests that the MixedBefore2's beginRender method should NOT be called. So, in MixedBefore2's beginRender method, an IllegalStateException is thrown. If you start up the app, and go to the page, you will see that the IllegalStateException is thrown. As Nick pointed out, either the documentation needs to be fixed, or the code needs to be fixed, but they are clearly at odds.
        Hide
        Robert Zeigler added a comment -

        Nick is right: the docs are pretty clear that subsequent methods in the same phase should be aborted, but they aren't; at least, subsequent mixin methods for the same phase aren't. I'll attach a sample project that illustrates the issue, generated with the quickstart archetype, so you can mvn jetty:run it.

        Show
        Robert Zeigler added a comment - Nick is right: the docs are pretty clear that subsequent methods in the same phase should be aborted, but they aren't; at least, subsequent mixin methods for the same phase aren't. I'll attach a sample project that illustrates the issue, generated with the quickstart archetype, so you can mvn jetty:run it.
        Nick Westgate made changes -
        Resolution Duplicate [ 3 ]
        Status Closed [ 6 ] Reopened [ 4 ]
        Hide
        Nick Westgate added a comment -

        This issue is not a duplicate of TAPESTRY-1805.

        In a nutshell, it's the answer to the following question:

        My component returns true for beforeRenderBody().
        Should this prevent all other beforeRenderBody events from executing?

        If so, it doesn't, and is a bug. If not, the docs need to restate the short-circuiting functionality.
        Seems it's a bug is because of the comment next to the exception raised:
        // Given that this method is only invoked from code
        // that is generated at runtime and proven to be correct,
        // this should never, ever happen. But what the hell,
        // let's check anyway.

        Cheers,
        Nick.

        Show
        Nick Westgate added a comment - This issue is not a duplicate of TAPESTRY-1805 . In a nutshell, it's the answer to the following question: My component returns true for beforeRenderBody(). Should this prevent all other beforeRenderBody events from executing? If so, it doesn't, and is a bug. If not, the docs need to restate the short-circuiting functionality. Seems it's a bug is because of the comment next to the exception raised: // Given that this method is only invoked from code // that is generated at runtime and proven to be correct, // this should never, ever happen. But what the hell, // let's check anyway. Cheers, Nick.
        Howard M. Lewis Ship made changes -
        Field Original Value New Value
        Status Open [ 1 ] Closed [ 6 ]
        Assignee Howard M. Lewis Ship [ hlship ]
        Resolution Duplicate [ 3 ]
        Hide
        Howard M. Lewis Ship added a comment -

        Duplicate of TAPESTRY-1805.

        Show
        Howard M. Lewis Ship added a comment - Duplicate of TAPESTRY-1805 .
        Hide
        Nick Westgate added a comment - - edited

        Sorry, I should have given an example, but at the time I thought the bug was pretty clear.
        Here's what I posted about it to the dev list over 2 months ago:

        Subject: T5: Clarification on render phase component/mixin conflict behaviour
        Newsgroups: gmane.comp.jakarta.tapestry.devel
        http://article.gmane.org/gmane.comp.jakarta.tapestry.devel/20864/
        Date: 2007-07-20 10:22:32 GMT (9 weeks, 1 day, 16 hours and 28 minutes ago)

        Hi.

        The component rendering guide suggests ordering and short-circuiting
        should prevent components and mixins using the same render phase from
        crossing swords, so to speak.

        Basically I've got a case where this doesn't seem to work out:

        AbstractField has a @Mixin DiscardBody.
        @Mixin DiscardBody returns false for beforeRenderBody().
        (Note that @Mixin DiscardBody has a @MixinAfter.)

        My component derives from AbstractField.
        My component returns true for beforeRenderBody().

        According to the docs I expect my component to short-circuit the
        phase and prevent DiscardBody's beforeRenderBody() from being invoked,
        but instead an exception is thrown. (Full stack trace appended below.)

        My expectation seems reasonable, especially considering the comment
        before the exception raising code, but which is correct?

        // Given that this method is only invoked from code
        // that is generated at runtime and proven to be correct,
        // this should never, ever happen. But what the hell,
        // let's check anyway.

        Cheers,
        Nick.

        java.lang.IllegalStateException

        Can not store result from invoking method org.apache.tapestry.corelib.
        mixins.DiscardBody.beforeRenderBody() (at DiscardBody.java:31), because
        an event result value has already been obtained from some other event
        handler method.

        Stack trace

        • org.apache.tapestry.internal.services.EventImpl.storeResult(EventImpl.java:58)
        • org.apache.tapestry.corelib.mixins.DiscardBody.beforeRenderBody(DiscardBody.java)
        • org.apache.tapestry.internal.structure.ComponentPageElementImpl$8$1.run(ComponentPageElementImpl.java:283)
        • org.apache.tapestry.internal.structure.ComponentPageElementImpl.invoke(ComponentPageElementImpl.java:931)
        • org.apache.tapestry.internal.structure.ComponentPageElementImpl.access$100(ComponentPageElementImpl.java:69)
        • org.apache.tapestry.internal.structure.ComponentPageElementImpl$8.render(ComponentPageElementImpl.java:287)
        • org.apache.tapestry.internal.services.RenderQueueImpl.run(RenderQueueImpl.java:57)
        • org.apache.tapestry.internal.services.PageMarkupRendererImpl.renderPageMarkup(PageMarkupRendererImpl.java:40)
        • org.apache.tapestry.internal.services.PageResponseRendererImpl.renderPageResponse(PageResponseRendererImpl.java:71)
        • org.apache.tapestry.internal.services.PageRenderRequestHandlerImpl.handle(PageRenderRequestHandlerImpl.java:81)
        • org.apache.tapestry.internal.services.PageRenderDispatcher.dispatch(PageRenderDispatcher.java:72)
        • org.apache.tapestry.services.TapestryModule$12.service(TapestryModule.java:1066)
        • jp.co.key_planning.kzoku.tapestry.services.AppModule$4.service(AppModule.java:211)
        • jp.co.key_planning.kzoku.tapestry.services.AppModule$6.service(AppModule.java:274)
        • org.apache.tapestry.internal.services.LocalizationFilter.service(LocalizationFilter.java:43)
        • org.apache.tapestry.services.TapestryModule$2.service(TapestryModule.java:657)
        • org.apache.tapestry.internal.services.StaticFilesFilter.service(StaticFilesFilter.java:63)
        • org.apache.tapestry.internal.services.CheckForUpdatesFilter$2.invoke(CheckForUpdatesFilter.java:97)
        • org.apache.tapestry.internal.services.CheckForUpdatesFilter$2.invoke(CheckForUpdatesFilter.java:88)
        • org.apache.tapestry.ioc.internal.util.ConcurrentBarrier.withRead(ConcurrentBarrier.java:77)
        • org.apache.tapestry.internal.services.CheckForUpdatesFilter.service(CheckForUpdatesFilter.java:110)
        • org.apache.tapestry.services.TapestryModule$11.service(TapestryModule.java:1044)
        • org.apache.tapestry.TapestryFilter.doFilter(TapestryFilter.java:135)
        • org.mortbay.jetty.servlet.WebApplicationHandler$CachedChain.doFilter(WebApplicationHandler.java:821)
        • org.mortbay.jetty.servlet.WebApplicationHandler.dispatch(WebApplicationHandler.java:471)
        • org.mortbay.jetty.servlet.ServletHandler.handle(ServletHandler.java:568)
        • org.mortbay.http.HttpContext.handle(HttpContext.java:1530)
        • org.mortbay.jetty.servlet.WebApplicationContext.handle(WebApplicationContext.java:633)
        • org.mortbay.http.HttpContext.handle(HttpContext.java:1482)
        • org.mortbay.http.HttpServer.service(HttpServer.java:909)
        • org.mortbay.http.HttpConnection.service(HttpConnection.java:820)
        • org.mortbay.http.HttpConnection.handleNext(HttpConnection.java:986)
        • org.mortbay.http.HttpConnection.handle(HttpConnection.java:837)
        • org.mortbay.http.SocketListener.handleConnection(SocketListener.java:245)
        • org.mortbay.util.ThreadedServer.handle(ThreadedServer.java:357)
        • org.mortbay.util.ThreadPool$PoolThread.run(ThreadPool.java:534)
        Show
        Nick Westgate added a comment - - edited Sorry, I should have given an example, but at the time I thought the bug was pretty clear. Here's what I posted about it to the dev list over 2 months ago: Subject: T5: Clarification on render phase component/mixin conflict behaviour Newsgroups: gmane.comp.jakarta.tapestry.devel http://article.gmane.org/gmane.comp.jakarta.tapestry.devel/20864/ Date: 2007-07-20 10:22:32 GMT (9 weeks, 1 day, 16 hours and 28 minutes ago) Hi. The component rendering guide suggests ordering and short-circuiting should prevent components and mixins using the same render phase from crossing swords, so to speak. Basically I've got a case where this doesn't seem to work out: AbstractField has a @Mixin DiscardBody. @Mixin DiscardBody returns false for beforeRenderBody(). (Note that @Mixin DiscardBody has a @MixinAfter.) My component derives from AbstractField. My component returns true for beforeRenderBody(). According to the docs I expect my component to short-circuit the phase and prevent DiscardBody's beforeRenderBody() from being invoked, but instead an exception is thrown. (Full stack trace appended below.) My expectation seems reasonable, especially considering the comment before the exception raising code, but which is correct? // Given that this method is only invoked from code // that is generated at runtime and proven to be correct, // this should never, ever happen. But what the hell, // let's check anyway. Cheers, Nick. java.lang.IllegalStateException Can not store result from invoking method org.apache.tapestry.corelib. mixins.DiscardBody.beforeRenderBody() (at DiscardBody.java:31), because an event result value has already been obtained from some other event handler method. Stack trace org.apache.tapestry.internal.services.EventImpl.storeResult(EventImpl.java:58) org.apache.tapestry.corelib.mixins.DiscardBody.beforeRenderBody(DiscardBody.java) org.apache.tapestry.internal.structure.ComponentPageElementImpl$8$1.run(ComponentPageElementImpl.java:283) org.apache.tapestry.internal.structure.ComponentPageElementImpl.invoke(ComponentPageElementImpl.java:931) org.apache.tapestry.internal.structure.ComponentPageElementImpl.access$100(ComponentPageElementImpl.java:69) org.apache.tapestry.internal.structure.ComponentPageElementImpl$8.render(ComponentPageElementImpl.java:287) org.apache.tapestry.internal.services.RenderQueueImpl.run(RenderQueueImpl.java:57) org.apache.tapestry.internal.services.PageMarkupRendererImpl.renderPageMarkup(PageMarkupRendererImpl.java:40) org.apache.tapestry.internal.services.PageResponseRendererImpl.renderPageResponse(PageResponseRendererImpl.java:71) org.apache.tapestry.internal.services.PageRenderRequestHandlerImpl.handle(PageRenderRequestHandlerImpl.java:81) org.apache.tapestry.internal.services.PageRenderDispatcher.dispatch(PageRenderDispatcher.java:72) org.apache.tapestry.services.TapestryModule$12.service(TapestryModule.java:1066) jp.co.key_planning.kzoku.tapestry.services.AppModule$4.service(AppModule.java:211) jp.co.key_planning.kzoku.tapestry.services.AppModule$6.service(AppModule.java:274) org.apache.tapestry.internal.services.LocalizationFilter.service(LocalizationFilter.java:43) org.apache.tapestry.services.TapestryModule$2.service(TapestryModule.java:657) org.apache.tapestry.internal.services.StaticFilesFilter.service(StaticFilesFilter.java:63) org.apache.tapestry.internal.services.CheckForUpdatesFilter$2.invoke(CheckForUpdatesFilter.java:97) org.apache.tapestry.internal.services.CheckForUpdatesFilter$2.invoke(CheckForUpdatesFilter.java:88) org.apache.tapestry.ioc.internal.util.ConcurrentBarrier.withRead(ConcurrentBarrier.java:77) org.apache.tapestry.internal.services.CheckForUpdatesFilter.service(CheckForUpdatesFilter.java:110) org.apache.tapestry.services.TapestryModule$11.service(TapestryModule.java:1044) org.apache.tapestry.TapestryFilter.doFilter(TapestryFilter.java:135) org.mortbay.jetty.servlet.WebApplicationHandler$CachedChain.doFilter(WebApplicationHandler.java:821) org.mortbay.jetty.servlet.WebApplicationHandler.dispatch(WebApplicationHandler.java:471) org.mortbay.jetty.servlet.ServletHandler.handle(ServletHandler.java:568) org.mortbay.http.HttpContext.handle(HttpContext.java:1530) org.mortbay.jetty.servlet.WebApplicationContext.handle(WebApplicationContext.java:633) org.mortbay.http.HttpContext.handle(HttpContext.java:1482) org.mortbay.http.HttpServer.service(HttpServer.java:909) org.mortbay.http.HttpConnection.service(HttpConnection.java:820) org.mortbay.http.HttpConnection.handleNext(HttpConnection.java:986) org.mortbay.http.HttpConnection.handle(HttpConnection.java:837) org.mortbay.http.SocketListener.handleConnection(SocketListener.java:245) org.mortbay.util.ThreadedServer.handle(ThreadedServer.java:357) org.mortbay.util.ThreadPool$PoolThread.run(ThreadPool.java:534)
        Hide
        Howard M. Lewis Ship added a comment -

        Could you be a bit more specific over what's broken?

        Show
        Howard M. Lewis Ship added a comment - Could you be a bit more specific over what's broken?
        Nick Westgate created issue -

          People

          • Assignee:
            Howard M. Lewis Ship
            Reporter:
            Nick Westgate
          • Votes:
            2 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development