Uploaded image for project: 'Wicket'
  1. Wicket
  2. WICKET-4012

Component's onAfterRender() is called so many times as it is depth in the component tree + 1

    Details

    • Type: Bug
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 1.5-RC7
    • Fix Version/s: 1.5.1
    • Component/s: wicket
    • Labels:
      None

      Description

      org.apache.wicket.Component.afterRender() calls org.apache.wicket.Component.onAfterRenderChildren() which for MarkupContainers calls afterRender() for its children.

      So for code like:

      WebMarkupContainer comp1 = new WebMarkupContainer("c1");
      add(comp1);

      WebMarkupContainer comp2 = new WebMarkupContainer("c2");
      comp1.add(comp2);

      WebMarkupContainer comp3 = new WebMarkupContainer("c3") {

      @Override
      protected void onAfterRender()

      { super.onAfterRender(); System.err.println("called"); }

      };
      comp2.add(comp3);

      you'll see "called" printed 4 times in a single request.

      Additionally I think onAfterRenderChildren() should be called before onAfterRender() in Component.afterRender(). The flow should be first-in last-out: onBeforeRender > onBeforeRenderChildren > onAfterRenderChildren > onAfterRender,

      1. wicket-4012.tar.gz
        18 kB
        Martin Grigorov
      2. detachedTwice.patch
        5 kB
        Martin Grigorov
      3. WICKET-4012-afterRender.patch
        2 kB
        Martin Grigorov

        Activity

        Hide
        ivaynberg Igor Vaynberg added a comment -

        looks ok

        Show
        ivaynberg Igor Vaynberg added a comment - looks ok
        Hide
        mgrigorov Martin Grigorov added a comment -

        Attaching a patch for onAfterRender() problem.
        Now instead of calling onAfterRender() on the children components which normally already had executed this method we just set RENDERING_FLAG to false. This is needed because of two reasons:
        1) this flag is being set even for invisible components (see org.apache.wicket.Component.internalRender(), line 2320 and the check in 2332)
        2) in Ajax request Enclosure needs to update this flag for its child component. The specifics are not very clear to me.

        Show
        mgrigorov Martin Grigorov added a comment - Attaching a patch for onAfterRender() problem. Now instead of calling onAfterRender() on the children components which normally already had executed this method we just set RENDERING_FLAG to false. This is needed because of two reasons: 1) this flag is being set even for invisible components (see org.apache.wicket.Component.internalRender(), line 2320 and the check in 2332) 2) in Ajax request Enclosure needs to update this flag for its child component. The specifics are not very clear to me.
        Hide
        mgrigorov Martin Grigorov added a comment -

        The solution for the problem with onAfterRender() is to remove org.apache.wicket.MarkupContainer.onAfterRenderChildren().
        There is no need of this method. Component.afterRender() is called right after internalRender() in Component.render().
        Parent's afterRender() comes after its children afterRender() so there is no reason to call it again.

        But now there are 4 tests for Enclosures which fail...

        Show
        mgrigorov Martin Grigorov added a comment - The solution for the problem with onAfterRender() is to remove org.apache.wicket.MarkupContainer.onAfterRenderChildren(). There is no need of this method. Component.afterRender() is called right after internalRender() in Component.render(). Parent's afterRender() comes after its children afterRender() so there is no reason to call it again. But now there are 4 tests for Enclosures which fail...
        Hide
        mgrigorov Martin Grigorov added a comment -

        Attaching the patch for onDetach().
        According to Igor we can ignore that because onDetach() is noop most of the time or just detaches some non-default models in the rare cases when it is used.
        I'm attaching the patch just in case it may be needed some day. It should be cleaned if ever applied.

        Show
        mgrigorov Martin Grigorov added a comment - Attaching the patch for onDetach(). According to Igor we can ignore that because onDetach() is noop most of the time or just detaches some non-default models in the rare cases when it is used. I'm attaching the patch just in case it may be needed some day. It should be cleaned if ever applied.
        Hide
        mgrigorov Martin Grigorov added a comment -

        org.apache.wicket.ComponentTest.detachPage() should be improved too.
        It checks whether onDetach() is called at least once, but it seems OK to be called 2+ times in a request.

        Show
        mgrigorov Martin Grigorov added a comment - org.apache.wicket.ComponentTest.detachPage() should be improved too. It checks whether onDetach() is called at least once, but it seems OK to be called 2+ times in a request.
        Hide
        mgrigorov Martin Grigorov added a comment -

        All scheduled pages are "touched" during their construction (see the call to init() at org.apache.wicket.Page.Page(PageParameters, IModel<?>)). At the end of the request all touched pages are detached in org.apache.wicket.page.RequestAdapter.commitRequest() and only the stateful ones are stored in the page stores.

        I'd be grateful for some help with onAfterRender() problem.

        Show
        mgrigorov Martin Grigorov added a comment - All scheduled pages are "touched" during their construction (see the call to init() at org.apache.wicket.Page.Page(PageParameters, IModel<?>)). At the end of the request all touched pages are detached in org.apache.wicket.page.RequestAdapter.commitRequest() and only the stateful ones are stored in the page stores. I'd be grateful for some help with onAfterRender() problem.
        Hide
        dashorst Martijn Dashorst added a comment -

        The latter is because you can have many page instances scheduled during a request, and they all should be detached: hence RenderPageRequestHandler.detach().

        Show
        dashorst Martijn Dashorst added a comment - The latter is because you can have many page instances scheduled during a request, and they all should be detached: hence RenderPageRequestHandler.detach().
        Hide
        mgrigorov Martin Grigorov added a comment -

        The reason for onDetach called twice is org.apache.wicket.request.handler.RenderPageRequestHandler.detach(IRequestCycle). It calls pageInstance.detach() and later org.apache.wicket.page.RequestAdapter.commitRequest() also calls page.detach().

        Show
        mgrigorov Martin Grigorov added a comment - The reason for onDetach called twice is org.apache.wicket.request.handler.RenderPageRequestHandler.detach(IRequestCycle). It calls pageInstance.detach() and later org.apache.wicket.page.RequestAdapter.commitRequest() also calls page.detach().
        Hide
        mgrigorov Martin Grigorov added a comment -

        In 1.4 both onAfterRender and onDetach are called just once per component. So this is definitely a bug.

        Show
        mgrigorov Martin Grigorov added a comment - In 1.4 both onAfterRender and onDetach are called just once per component. So this is definitely a bug.
        Hide
        mgrigorov Martin Grigorov added a comment -

        The same pattern is used for onDetach().

        Modify the quickstart to:
        WebMarkupContainer comp3 = new WebMarkupContainer("c3") {

        @Override
        protected void onAfterRender()

        { super.onAfterRender(); System.err.println("called"); }

        @Override
        protected void onDetach()

        { super.onDetach(); System.err.println("onDetach"); }

        };

        And you'll see onDetach twice per request. Again the parent component calls onDetach for all its children and this call is recursive.

        Is there a hidden purpose here ?

        Show
        mgrigorov Martin Grigorov added a comment - The same pattern is used for onDetach(). Modify the quickstart to: WebMarkupContainer comp3 = new WebMarkupContainer("c3") { @Override protected void onAfterRender() { super.onAfterRender(); System.err.println("called"); } @Override protected void onDetach() { super.onDetach(); System.err.println("onDetach"); } }; And you'll see onDetach twice per request. Again the parent component calls onDetach for all its children and this call is recursive. Is there a hidden purpose here ?
        Hide
        mgrigorov Martin Grigorov added a comment -

        I tried to add new request flag and call #afterRender() just once but then 4 tests failed. Needs some more investigation.

        Show
        mgrigorov Martin Grigorov added a comment - I tried to add new request flag and call #afterRender() just once but then 4 tests failed. Needs some more investigation.
        Hide
        mgrigorov Martin Grigorov added a comment -

        A quickstart showing the problem

        Show
        mgrigorov Martin Grigorov added a comment - A quickstart showing the problem

          People

          • Assignee:
            mgrigorov Martin Grigorov
            Reporter:
            mgrigorov Martin Grigorov
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development