Wicket
  1. Wicket
  2. WICKET-3972

wicket:message attribute results in "The component was rendered already" error

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.5-RC5.1
    • Fix Version/s: 1.5-RC6
    • Component/s: wicket
    • Labels:
      None
    • Environment:
      Linux Ubuntu, Tomcat 6, Java 6

      Description

      Seems like there is a problem with generation of tag and component IDs when using wicket:message as an attribute for tags inside ListView.
      The quickstart webapp is attached to this ticket. There are two pages in this webapp: HomePage with a ListView containing tags with wicket:message attribute and two tags with wicket:message attribute outside the ListView. If a user goes to the next page (SecondPage) using the link on HomePage and then goes back to HomePage using the link on SecondPage, the exception mentioned in the subject occurs.
      Seems that the ID generation for tags and components is wrong: the ComponentTag-s are cached together with the corresponding Markup and then reused for later renderings. But the problem is, that those ComponentTag-s are mutable (at least in case of tags with wicket:message attribute) and their IDs are changed on every rendering and this produces ID conflicts when MarkupContainer.renderNext tries to find or create components corresponding to the tags.
      Interesting is, that we can reproduce this bug only in DEVELOPMENT mode. In DEPLOYMENT mode everything seems to work. The solution would be to make ComponentTags immutable and do not allow to change them, but to create copies (there is mutable method in ComponentTag, which is used in some cases).

      1. wicket-message-index.patch
        2 kB
        Peter Ertl
      2. quickstart.zip
        22 kB
        Sergiy Barlabanov

        Activity

        Hide
        Juergen Donnerstag added a comment -

        That is only partially true. The ComponentTag exposed to "normal" users via onComponentTag() etc. is a copy and can be modified without doing harm to the core. Handlers are already something for advanced users. But I accept that it'd be nice to throw an exception in cases like setId(). That should actually be easy to implement.

        Show
        Juergen Donnerstag added a comment - That is only partially true. The ComponentTag exposed to "normal" users via onComponentTag() etc. is a copy and can be modified without doing harm to the core. Handlers are already something for advanced users. But I accept that it'd be nice to throw an exception in cases like setId(). That should actually be easy to implement.
        Hide
        Sergiy Barlabanov added a comment -

        thanks for the fix. Will wait till RC6 .
        Though I think, that making Markups immutable would be a more clean solution (requiring more code ). There is no real need to reassign tag ids on every page/component render. The mutability of markup is a potential source of future errors, since component markups and ComponentTag objects contained inside are reused multiple times in different requests resulting in race conditions like the one described in this bug report.

        Show
        Sergiy Barlabanov added a comment - thanks for the fix. Will wait till RC6 . Though I think, that making Markups immutable would be a more clean solution (requiring more code ). There is no real need to reassign tag ids on every page/component render. The mutability of markup is a potential source of future errors, since component markups and ComponentTag objects contained inside are reused multiple times in different requests resulting in race conditions like the one described in this bug report.
        Hide
        Peter Ertl added a comment -

        Wow, I was really surprised that just leaving off a single line will do the trick. Impressive!

        At least I learned a lot more about handlers and resolvers, thanks guys!!

        Show
        Peter Ertl added a comment - Wow, I was really surprised that just leaving off a single line will do the trick. Impressive! At least I learned a lot more about handlers and resolvers, thanks guys!!
        Hide
        Martin Grigorov added a comment -

        Applied Juergen's suggestion. It solves the problem.

        Show
        Martin Grigorov added a comment - Applied Juergen's suggestion. It solves the problem.
        Hide
        Juergen Donnerstag added a comment -

        Unfortunately it doesn't work. I wish it would, since it's simple than what we need to do today. There are several issues with this approach:

        • which counter do you want to use? A new handler is created with every markup. The Page is not available. May be the Session, but you don't want to create it for stateless apps. The App / MarkupFactory?
        • You couldn't even use a counter per Handler if the Handler wouldn't get re-created, or use the resource stream or whatever, since the number scheme would only work if you can guarantee that the markup/Component is always used with the same Page only. But of course Components can be added to as many different Pages/Containers as you like. And every Page has a different sequence in which it loads its components. You inevitably will have clashes, except you choose an application scoped counter. But that is inpracticable, since it will overrun for large or busy apps, or apps which are in prod for a long time.
        • Because of transparent containers you cannot assume the much smuch smaller scope of a container for possible clashes, but to be safe, the complete page.

        Did you try if the quickstart provided by Sergiy works if you remove tag.setId(xx) from WicketMessageTagHandler.resolver()? The invocation of tag.setId(xxx) should really not be there.

        I think the convention is fine. The issue is that we don't enforce it in the code.

        Show
        Juergen Donnerstag added a comment - Unfortunately it doesn't work. I wish it would, since it's simple than what we need to do today. There are several issues with this approach: which counter do you want to use? A new handler is created with every markup. The Page is not available. May be the Session, but you don't want to create it for stateless apps. The App / MarkupFactory? You couldn't even use a counter per Handler if the Handler wouldn't get re-created, or use the resource stream or whatever, since the number scheme would only work if you can guarantee that the markup/Component is always used with the same Page only. But of course Components can be added to as many different Pages/Containers as you like. And every Page has a different sequence in which it loads its components. You inevitably will have clashes, except you choose an application scoped counter. But that is inpracticable, since it will overrun for large or busy apps, or apps which are in prod for a long time. Because of transparent containers you cannot assume the much smuch smaller scope of a container for possible clashes, but to be safe, the complete page. Did you try if the quickstart provided by Sergiy works if you remove tag.setId(xx) from WicketMessageTagHandler.resolver()? The invocation of tag.setId(xxx) should really not be there. I think the convention is fine. The issue is that we don't enforce it in the code.
        Hide
        Peter Ertl added a comment - - edited

        @Jürgen: Thanks for the feedback

        I think the problem can be solved by not assigning a temporary component tag id ('message_attr') but a permanent one including a counter to the component tags with 'wicket:message' attribute.

        currently we have the situation that 3 component tags with 'wicket:message' are located in the markup but only two of them get resolved on first render of HomePage

        component tag#1 (id = 'message_attr') – invisible since list view is empty, so WicketMessageTagHandler#resolve() is not invoked and temporary id stays
        component tag#2 (id = '_message_attr_3')
        component tag#3 (id = '_message_attr_4')

        -> go to SecondPage

        -> go to HomePage with now visible ListView caused by page parameter

        resolve now assigns a permanent id to the first component which will cause an identifier collision

        component tag#1 (id = '_message_attr_4') -> new permanent id assigned, will refer to component that has been attached to component tag #3 recently
        component tag#2 (id = '_message_attr_3') -> since the component tag has a related component resolve will not be invoked and the old id stays
        component tag#3 (id = '_message_attr_4') -> since the component tag has a related component resolve will not be invoked and the old id stays; wicket will again render component that has accidentally already been rendered by component tag #1 and causes the 'component was already rendered' error

        new patch included...

        Show
        Peter Ertl added a comment - - edited @Jürgen: Thanks for the feedback I think the problem can be solved by not assigning a temporary component tag id (' message_attr ') but a permanent one including a counter to the component tags with 'wicket:message' attribute. currently we have the situation that 3 component tags with 'wicket:message' are located in the markup but only two of them get resolved on first render of HomePage component tag#1 (id = ' message_attr ') – invisible since list view is empty, so WicketMessageTagHandler#resolve() is not invoked and temporary id stays component tag#2 (id = '_message_attr_3') component tag#3 (id = '_message_attr_4') -> go to SecondPage -> go to HomePage with now visible ListView caused by page parameter resolve now assigns a permanent id to the first component which will cause an identifier collision component tag#1 (id = '_message_attr_4') -> new permanent id assigned, will refer to component that has been attached to component tag #3 recently component tag#2 (id = '_message_attr_3') -> since the component tag has a related component resolve will not be invoked and the old id stays component tag#3 (id = '_message_attr_4') -> since the component tag has a related component resolve will not be invoked and the old id stays; wicket will again render component that has accidentally already been rendered by component tag #1 and causes the 'component was already rendered' error new patch included...
        Hide
        Juergen Donnerstag added a comment -

        I don't think your observations are completely right. *Handlers are executed while loading the markup and before the markup gets cached. They get re-used for multiple instances of typically (but necessarily) the same Components. The Page instance is not known. The main purpose of *Handlers are to identify tags which Wicket's render process must somehow handle.

        *Resolvers are used during the render process to automatically create (or find/resolve) components which know how to handle the tag. Many *Resolvers create auto-components, which are typically transparent, and which are automatically removed again after the render process. They get re-created with the next render cycle. The Page is known during the render process. And because the page auto-index gets incremented, it should never happen that for the same page instance components with the same id are created. I don't think we need some other mechanism. A *Resolver should never change the tag id, only the *Handler may.

        The solution is much simpler. WicketMessageTagHandler.resolver() should not call tag.setId()

        Show
        Juergen Donnerstag added a comment - I don't think your observations are completely right. *Handlers are executed while loading the markup and before the markup gets cached. They get re-used for multiple instances of typically (but necessarily) the same Components. The Page instance is not known. The main purpose of *Handlers are to identify tags which Wicket's render process must somehow handle. *Resolvers are used during the render process to automatically create (or find/resolve) components which know how to handle the tag. Many *Resolvers create auto-components, which are typically transparent, and which are automatically removed again after the render process. They get re-created with the next render cycle. The Page is known during the render process. And because the page auto-index gets incremented, it should never happen that for the same page instance components with the same id are created. I don't think we need some other mechanism. A *Resolver should never change the tag id, only the *Handler may. The solution is much simpler. WicketMessageTagHandler.resolver() should not call tag.setId()
        Hide
        Peter Ertl added a comment -

        For component tags that not necessarily relate to a wicket component (tag id <> component id) we need to place auto index somewhere else but not in page. I will attach a patch which puts the auto index right inside MarkupResourceStream. I think this is the technically right place. However I don't know if this is ugly or the way to solve it, so feedback is welcome...

        Show
        Peter Ertl added a comment - For component tags that not necessarily relate to a wicket component (tag id <> component id) we need to place auto index somewhere else but not in page. I will attach a patch which puts the auto index right inside MarkupResourceStream. I think this is the technically right place. However I don't know if this is ugly or the way to solve it, so feedback is welcome...
        Hide
        Peter Ertl added a comment -

        replace above 'component id' with 'tag id'

        Show
        Peter Ertl added a comment - replace above 'component id' with 'tag id'
        Hide
        Peter Ertl added a comment -

        Short summary on what I found so far:

        The problem lies within WicketMessageTagHandler, WicketMessageResolver and the behavior of Page.getAutoIndex().

        When rendering HomePage for the 1st time WicketMessageResolver resolves the 'wicket:message' attributes and sets the component id of the affected components to 'message_attr'.

        WicketMessageTagHandler will detect all component with 'message_attr' and add an auto index. The auto index starts with zero when rendering begins and returns unique id's for dynamically creating unique component ids.

        So after the first render we have the first input tag resolved with id = '_message_attr_4' (the one that causes the crash later). Now we redirect to SecondPage and return. Since the list view will create another tag with 'wicket:message' it will receive ''_message_attr' as temporary component id. Again WicketMessageTagHandler tries to assign an auto index (which starts with zero every new render). In this example it gets index = 4 by chance (unlucky!) which already exists so we get a duplicate component id.

        The solution is probably to check if a component already exists when creating new component id's with auto index or to keep auto index incrementing over different page id's for the same page (however this will make stateless pages stateful).

        I keep on investigating...

        Show
        Peter Ertl added a comment - Short summary on what I found so far: The problem lies within WicketMessageTagHandler, WicketMessageResolver and the behavior of Page.getAutoIndex(). When rendering HomePage for the 1st time WicketMessageResolver resolves the 'wicket:message' attributes and sets the component id of the affected components to ' message_attr '. WicketMessageTagHandler will detect all component with ' message_attr ' and add an auto index. The auto index starts with zero when rendering begins and returns unique id's for dynamically creating unique component ids. So after the first render we have the first input tag resolved with id = '_message_attr_4' (the one that causes the crash later). Now we redirect to SecondPage and return. Since the list view will create another tag with 'wicket:message' it will receive ''_message_attr' as temporary component id. Again WicketMessageTagHandler tries to assign an auto index (which starts with zero every new render). In this example it gets index = 4 by chance (unlucky!) which already exists so we get a duplicate component id. The solution is probably to check if a component already exists when creating new component id's with auto index or to keep auto index incrementing over different page id's for the same page (however this will make stateless pages stateful). I keep on investigating...
        Hide
        Martin Grigorov added a comment -

        This is a hard one to track the bug...
        There are two stable wicket:message components and the other are dynamically generated depending on the request parameter value.
        If I disable any of them then the app works, but otherwise the 'link' component is not rendered, component use check runs, and for some reason during the rendering of the error page the error message gets replaced by "wicket:message with id '..' was already rendered".

        Show
        Martin Grigorov added a comment - This is a hard one to track the bug... There are two stable wicket:message components and the other are dynamically generated depending on the request parameter value. If I disable any of them then the app works, but otherwise the 'link' component is not rendered, component use check runs, and for some reason during the rendering of the error page the error message gets replaced by "wicket:message with id '..' was already rendered".

          People

          • Assignee:
            Juergen Donnerstag
            Reporter:
            Sergiy Barlabanov
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development