Wicket
  1. Wicket
  2. WICKET-3931

Component markup caching inconsistencies

    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

      Description

      In WICKET-3891 we found that Component#markup field is not being reset between requests. The problem is that this field is transient and it is null-ified only when the page is read from the second level page cache (see https://cwiki.apache.org/confluence/x/qIaoAQ). If the page instance is read from first level cache (http session) then its non-serialized version is used and the markup field value is still non-null.

      In WICKET-3891 this looked like a minor issue with the markup caching in development mode but actually this problem is valid even in production mode.
      See the attached application. When the panel's variation is changed every MarkupContainer inside still uses its old markup.

      1. variation.tgz
        21 kB
        Martin Grigorov
      2. WICKET-3931.patch
        3 kB
        Martin Grigorov
      3. WICKET-3931.patch
        4 kB
        Martin Grigorov

        Issue Links

          Activity

          Hide
          Martin Grigorov added a comment -

          Submitted the fix with r1151831.
          Re-worked a bit the previous version because MarkupFragment is not serializable.
          Please review and comment if there is something wrong.

          Show
          Martin Grigorov added a comment - Submitted the fix with r1151831. Re-worked a bit the previous version because MarkupFragment is not serializable. Please review and comment if there is something wrong.
          Hide
          Martin Grigorov added a comment -

          Here is the final patch. All tests and the variation quickstart work.
          I'll modify the quickstart to a unit test.

          Show
          Martin Grigorov added a comment - Here is the final patch. All tests and the variation quickstart work. I'll modify the quickstart to a unit test.
          Hide
          Juergen Donnerstag added a comment -

          First I was thinking that other components are affected as well, but only auto-components get their markup set via setMarkup(), which is why they don't need to know how to find their markup. But than I found this:

          MarkupContainer:
          void detachChildren()
          {
          ...
          // We need to keep InlineEnclosures for Ajax request handling.
          // TODO this is really ugly. Feature request for 1.5: change auto-component that
          // they don't need to be removed anymore.
          if (component.isAuto() && !(component instanceof InlineEnclosure))

          { children_remove(i); }

          which makes me belief that the problem is limited to InlineEnclosure only. I think we have two options. Either the way it's implemented in your patch (InlineEnclosure implements getMarkup()) or InlineEnclosure gets a variable markup which is not reset. Any variation change has no effect on InlineEnclosure ajax upates anyway, since the markup will not be reloaded under any circumstances.

          Pseudo code: InlineEnclosure
          // avoid all the extra info which come with IMF implementations
          private String markup

          IMF getMarkup()
          {
          IMF markup = super.getMarkup();

          // resolver has set markup upon auto-component creation. Local markup not yet set => remember
          if (markup != null && this.markup == null)

          { this.markup = markup.toString() }

          // markup == null because of detach, but we have our local copy => use copy to set markup
          else if (markup == null && this.markup != null)

          { markup = Markup.of(this.markup) setMarkup(markup) }

          return markup
          }

          Show
          Juergen Donnerstag added a comment - First I was thinking that other components are affected as well, but only auto-components get their markup set via setMarkup(), which is why they don't need to know how to find their markup. But than I found this: MarkupContainer: void detachChildren() { ... // We need to keep InlineEnclosures for Ajax request handling. // TODO this is really ugly. Feature request for 1.5: change auto-component that // they don't need to be removed anymore. if (component.isAuto() && !(component instanceof InlineEnclosure)) { children_remove(i); } which makes me belief that the problem is limited to InlineEnclosure only. I think we have two options. Either the way it's implemented in your patch (InlineEnclosure implements getMarkup()) or InlineEnclosure gets a variable markup which is not reset. Any variation change has no effect on InlineEnclosure ajax upates anyway, since the markup will not be reloaded under any circumstances. Pseudo code: InlineEnclosure // avoid all the extra info which come with IMF implementations private String markup IMF getMarkup() { IMF markup = super.getMarkup(); // resolver has set markup upon auto-component creation. Local markup not yet set => remember if (markup != null && this.markup == null) { this.markup = markup.toString() } // markup == null because of detach, but we have our local copy => use copy to set markup else if (markup == null && this.markup != null) { markup = Markup.of(this.markup) setMarkup(markup) } return markup }
          Hide
          Martin Grigorov added a comment -

          This is the best I was able to do so far.
          Now InlineEnclosure has a logic to find its markup for Ajax requests.

          The only problem is that now org.apache.wicket.markup.html.internal.AjaxEnclosureTest.testNestedInlineEnclosuresShouldToggleNormally() fails because all InlineEnclosures in a page have the same component id (InlineEnclosure-) and finding the markup is impossible.

          Show
          Martin Grigorov added a comment - This is the best I was able to do so far. Now InlineEnclosure has a logic to find its markup for Ajax requests. The only problem is that now org.apache.wicket.markup.html.internal.AjaxEnclosureTest.testNestedInlineEnclosuresShouldToggleNormally() fails because all InlineEnclosures in a page have the same component id (InlineEnclosure-) and finding the markup is impossible.
          Hide
          Martin Grigorov added a comment -

          The tests for InlineEnclosure fail with this change.
          I'll investigate.

          Show
          Martin Grigorov added a comment - The tests for InlineEnclosure fail with this change. I'll investigate.
          Hide
          Juergen Donnerstag added a comment -

          I guess it is the easiest and cleanest solution:
          pro: applies to all components (you can not miss one, even though it might not be needed)
          pro: reduced memory footprint.
          con: requires to load markup upon the next request. Though loading from MarkupCache is cheap. I don't expect any performance penalities.

          I wonder if we need to make sure that not by accident the markup gets re-loaded after detach. E.g. if a user first calls super.detach() but than uses getMarkup() for whatever reason in his onDetach(). Maybe it's better to cleanup the markup after onDetach() has been executed via iterating over the hierarchy again and calling setMarkup(null).

          It'd be good to add test case as well.

          Show
          Juergen Donnerstag added a comment - I guess it is the easiest and cleanest solution: pro: applies to all components (you can not miss one, even though it might not be needed) pro: reduced memory footprint. con: requires to load markup upon the next request. Though loading from MarkupCache is cheap. I don't expect any performance penalities. I wonder if we need to make sure that not by accident the markup gets re-loaded after detach. E.g. if a user first calls super.detach() but than uses getMarkup() for whatever reason in his onDetach(). Maybe it's better to cleanup the markup after onDetach() has been executed via iterating over the hierarchy again and calling setMarkup(null). It'd be good to add test case as well.
          Hide
          Martin Grigorov added a comment -

          Juergen, what do you think about this ?

          Show
          Martin Grigorov added a comment - Juergen, what do you think about this ?
          Hide
          Martin Grigorov added a comment -

          Index: wicket-core/src/main/java/org/apache/wicket/Component.java
          ===================================================================
          — wicket-core/src/main/java/org/apache/wicket/Component.java (revision 1150368)
          +++ wicket-core/src/main/java/org/apache/wicket/Component.java (working copy)
          @@ -3807,6 +3807,8 @@
          */
          protected void onDetach()

          { + markup = null; + setFlag(FLAG_DETACHING, false); }

          This fixes the problem. This way the markup is read from the MarkupCache at every request and it properly refreshes changes in DEV mode and changes in the component variation in DEPLOYMENT mode.
          The 'markup' variable is still 'transient' just in case.

          Show
          Martin Grigorov added a comment - Index: wicket-core/src/main/java/org/apache/wicket/Component.java =================================================================== — wicket-core/src/main/java/org/apache/wicket/Component.java (revision 1150368) +++ wicket-core/src/main/java/org/apache/wicket/Component.java (working copy) @@ -3807,6 +3807,8 @@ */ protected void onDetach() { + markup = null; + setFlag(FLAG_DETACHING, false); } This fixes the problem. This way the markup is read from the MarkupCache at every request and it properly refreshes changes in DEV mode and changes in the component variation in DEPLOYMENT mode. The 'markup' variable is still 'transient' just in case.
          Hide
          Martin Grigorov added a comment -

          A quickstart showing the problem.

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

            People

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

              Dates

              • Created:
                Updated:
                Resolved:

                Development