Wicket
  1. Wicket
  2. WICKET-4046

Touch pages only when they are returned from the data stores

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.5.0, 1.5.1
    • Fix Version/s: 1.5.1
    • Component/s: wicket
    • Labels:
      None

      Description

      The patch for WICKET-4021 can cause an unused instance of a page to be touched. This page is then stored by the page store, rather than the page used in the request. The attached patch cleans up PageProvider, by removing the touch call and makes the RequestAdapter search for a touched page first and only then load it from the page store.

      1. pageprovider-notouch.diff
        3 kB
        Emond Papegaaij
      2. WICKET-4046.patch
        9 kB
        Pedro Santos
      3. WICKET-4046-2.diff
        0.7 kB
        Emond Papegaaij
      4. WICKET-4046-3.diff
        0.7 kB
        Emond Papegaaij

        Issue Links

          Activity

          Emond Papegaaij created issue -
          Emond Papegaaij made changes -
          Field Original Value New Value
          Attachment pageprovider-notouch.diff [ 12494201 ]
          Emond Papegaaij made changes -
          Link This issue is related to WICKET-4021 [ WICKET-4021 ]
          Hide
          Pedro Santos added a comment -

          IIRC the touching logic in PageProvider prevents URLs to AJAX callbacks in page from get dirty.
          WICKET-3676 is an example of this problem, I'm preparing one test case preventing it.

          Show
          Pedro Santos added a comment - IIRC the touching logic in PageProvider prevents URLs to AJAX callbacks in page from get dirty. WICKET-3676 is an example of this problem, I'm preparing one test case preventing it.
          Hide
          Pedro Santos added a comment -

          The problem in WICKET-3676 was triggered by changes in AbstractPageManager and it is already prevented by PageIdPoliticTest.

          Show
          Pedro Santos added a comment - The problem in WICKET-3676 was triggered by changes in AbstractPageManager and it is already prevented by PageIdPoliticTest.
          Hide
          Pedro Santos added a comment -

          Hi Emond, how this behavior of multiple instances getting touched are a bug?
          Even if the touch logic gets removed from PageProvider, the accessed page from page store will keep being touched by other means, like by AbstractPageManager. It's important because one may want to get its reference calling Page#getPageReference for further access, in which case the framework needs to make sure it is already stored.

          Anyway I'm 1+ to remove the duplicated touch logic from PageProvider as the patch does and change this ticket from bug to improvement.

          About make the RequestAdapter search for a touched page first, I would suggest we continue this improvement discussion at WICKET-4050. This improvement was already discussed in devs mail list and postponed to 1.5.x series.

          Show
          Pedro Santos added a comment - Hi Emond, how this behavior of multiple instances getting touched are a bug? Even if the touch logic gets removed from PageProvider, the accessed page from page store will keep being touched by other means, like by AbstractPageManager. It's important because one may want to get its reference calling Page#getPageReference for further access, in which case the framework needs to make sure it is already stored. Anyway I'm 1+ to remove the duplicated touch logic from PageProvider as the patch does and change this ticket from bug to improvement. About make the RequestAdapter search for a touched page first, I would suggest we continue this improvement discussion at WICKET-4050 . This improvement was already discussed in devs mail list and postponed to 1.5.x series.
          Hide
          Emond Papegaaij added a comment -

          The proposed patch is intended to included as a whole. One part will not work without the other. The problem with the current behaviour is that since WICKET-4021, the page provider retrieves a page from the page store in isNewPageInstance. This instance is then touched by the AbstractPageManager, however the PageProvider does not keep hold of it. Next, in getPageInstance, a new instance of the same page is fetched from the page store. This instance is not touched, because another instance of the same page is already touched. This second instance is used in the request, but at the end of the request, the first instance is stored in the page map.

          As you can see, the proposed patch is an improvement to PageProvider (removing responsibility from that class that shouldn't have been there), a performance optimisation (do not fetch the same page from the page store multiple times) and a bug fix (the page used in the request is now stored in the page store, rather than a stale instance).

          Show
          Emond Papegaaij added a comment - The proposed patch is intended to included as a whole. One part will not work without the other. The problem with the current behaviour is that since WICKET-4021 , the page provider retrieves a page from the page store in isNewPageInstance. This instance is then touched by the AbstractPageManager, however the PageProvider does not keep hold of it. Next, in getPageInstance, a new instance of the same page is fetched from the page store. This instance is not touched, because another instance of the same page is already touched. This second instance is used in the request, but at the end of the request, the first instance is stored in the page map. As you can see, the proposed patch is an improvement to PageProvider (removing responsibility from that class that shouldn't have been there), a performance optimisation (do not fetch the same page from the page store multiple times) and a bug fix (the page used in the request is now stored in the page store, rather than a stale instance).
          Martin Grigorov made changes -
          Summary Multiple instances of same page in request and wrong one touched Touch pages only when they are returned from the data stores
          Issue Type Bug [ 1 ] Improvement [ 4 ]
          Assignee Martin Grigorov [ mgrigorov ]
          Affects Version/s 1.5.0 [ 12315432 ]
          Hide
          Martin Grigorov added a comment -

          The patch is applied!
          Thanks!

          Show
          Martin Grigorov added a comment - The patch is applied! Thanks!
          Martin Grigorov made changes -
          Status Open [ 1 ] Resolved [ 5 ]
          Fix Version/s 1.5.1 [ 12315430 ]
          Resolution Fixed [ 1 ]
          Hide
          Pedro Santos added a comment -

          Sure, I see the bug now. I think the bug can be solved just in PageProvider, by caching its restored page by id. Let me also propose a patch with a this bug fix plus test case.

          Show
          Pedro Santos added a comment - Sure, I see the bug now. I think the bug can be solved just in PageProvider, by caching its restored page by id. Let me also propose a patch with a this bug fix plus test case.
          Hide
          Pedro Santos added a comment -

          Emond's optimization in RequestAdapter fixed the bug by preventing the second deserialization. But PageProvider still needs to be improved to not try to find a page for the same id more than one time, and always test the page class after get it from page store.

          attached patch:

          • also fix the double deserialization in PageProvider
          • prevents a different page with the same page id in PageProvider from affect the isNewPageInstance state.
          • test case preventing the bug
          Show
          Pedro Santos added a comment - Emond's optimization in RequestAdapter fixed the bug by preventing the second deserialization. But PageProvider still needs to be improved to not try to find a page for the same id more than one time, and always test the page class after get it from page store. attached patch: also fix the double deserialization in PageProvider prevents a different page with the same page id in PageProvider from affect the isNewPageInstance state. test case preventing the bug
          Pedro Santos made changes -
          Attachment WICKET-4046.patch [ 12494480 ]
          Hide
          Jeremy Thomerson added a comment -

          Just re-opening since Pedro attached something after it was closed... Don't want it to get lost.

          Show
          Jeremy Thomerson added a comment - Just re-opening since Pedro attached something after it was closed... Don't want it to get lost.
          Jeremy Thomerson made changes -
          Resolution Fixed [ 1 ]
          Status Resolved [ 5 ] Reopened [ 4 ]
          Emond Papegaaij made changes -
          Attachment WICKET-4046-2.diff [ 12494574 ]
          Hide
          Emond Papegaaij added a comment -

          The same effect can be achieved in PageProvider with this patch. Your testcases still pass. I don't think there is any need for a second reference to an IRequestablePage, which should always be te same page as 'pageInstance'.

          I did notice one more curious thing tho: getPageParameters() only works on the pageInstance field, not on getPageInstance. This means it will return null when the page has not yet been fetched from the store. I guess those references should be changed to getPageInstance().

          Show
          Emond Papegaaij added a comment - The same effect can be achieved in PageProvider with this patch. Your testcases still pass. I don't think there is any need for a second reference to an IRequestablePage, which should always be te same page as 'pageInstance'. I did notice one more curious thing tho: getPageParameters() only works on the pageInstance field, not on getPageInstance. This means it will return null when the page has not yet been fetched from the store. I guess those references should be changed to getPageInstance().
          Emond Papegaaij made changes -
          Attachment WICKET-4046-3.diff [ 12494588 ]
          Hide
          Martin Grigorov added a comment -

          Please re-open or comment if you see room for more optimizations.

          Show
          Martin Grigorov added a comment - Please re-open or comment if you see room for more optimizations.
          Martin Grigorov made changes -
          Status Reopened [ 4 ] Resolved [ 5 ]
          Resolution Fixed [ 1 ]
          Martin Grigorov committed 1171415 (1 file)
          Reviews: none

          WICKET-4046 Touch pages only when they are returned from the data stores

          Apply simplified version of Emond's patch '-3' - if this is not a new instance then the cached pageInstance is either non-null from construction time or has been just loaded from the stores.

            People

            • Assignee:
              Martin Grigorov
              Reporter:
              Emond Papegaaij
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development