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

Wicket does not handle non in-memory Httpsessions correctly

    Details

    • Type: Bug
    • Status: Resolved
    • Priority: Critical
    • Resolution: Fixed
    • Affects Version/s: 6.12.0
    • Fix Version/s: 6.14.0, 7.0.0-M1
    • Component/s: wicket
    • Labels:
      None

      Description

      There are two issues (related to WICKET-5390):

      1) The "Wicket-Session" (as well as WebSession) has internal state that is not reflected inside equals comparison (e.g. page id counter) since it does not override equals. This can leads in de/serialization szenarios to render two (Web)Sessions as unequal even if they are in fact the same.

      2) The "Wicket-Session" object is stored inside the HttpSession object only once via setAttribute. This might be persisted. In successive calls the internal state is changed (especially the page id counter) but the object is not "updated" via setAttribute. This leads to the problem, that the HttpSession implementation never knows something has changed and might miss to persist the change!

      This leads to duplicate and wrong assigned page ids and confussing errors.

      So the Wicket-(Web)Session should be changed in a way, that:
      a) every internal (non transient) state is used in equals comparisons
      b) each change to any (non transient) internal state causes a setAttribute to the underlying storage so it is notified about the change.

      We are hitting this problem with very obscure error messages, so I mark this as a blocker since it produces hard to track problems and currently holds us down to use Wicket with a persitent (clustered) session model.

      In "standard" configuration you very likely never hit this problem since the default PageStore implementation modifies the session so implicitly it gets its changes persited. But with alternative PageStore or stateless pages you still can hit the problem (and get really confused).

      1. WICKET-5473.patch
        0.7 kB
        Martin Grigorov

        Issue Links

          Activity

          Hide
          mgrigorov Martin Grigorov added a comment -

          Thanks for your help!

          Show
          mgrigorov Martin Grigorov added a comment - Thanks for your help!
          Hide
          laeubi Christoph Läubrich added a comment -

          I tried this patch and it solves the issue seen before.

          Show
          laeubi Christoph Läubrich added a comment - I tried this patch and it solves the issue seen before.
          Hide
          mgrigorov Martin Grigorov added a comment -

          If there are no issues found then it will be in 6.14.0.
          We try to release a new minor version every month.
          6.13.0 has been released in the beginning of this month so there are around a week or two until 6.14.0.

          But you can put
          public synchronized int nextSequenceValue()
          {
          if (isTemporary() == false)

          { dirty(); }
          return super.nextSequenceValue();
          }

          /**
          *
          * @return the next page id
          */
          public synchronized int nextPageId()
          {
          if (isTemporary() == false)
          { dirty(); }

          return super.nextPageId();
          }

          in YourSession.java and use it until 6.14.0 is out.

          Show
          mgrigorov Martin Grigorov added a comment - If there are no issues found then it will be in 6.14.0. We try to release a new minor version every month. 6.13.0 has been released in the beginning of this month so there are around a week or two until 6.14.0. But you can put public synchronized int nextSequenceValue() { if (isTemporary() == false) { dirty(); } return super.nextSequenceValue(); } /** * * @return the next page id */ public synchronized int nextPageId() { if (isTemporary() == false) { dirty(); } return super.nextPageId(); } in YourSession.java and use it until 6.14.0 is out.
          Hide
          laeubi Christoph Läubrich added a comment -

          Thanks for your investigation and the patch, I'll try this tomorrow!
          Can you tell (if the patch works) how long it normally takes untill it will find its way into an offical release?

          Show
          laeubi Christoph Läubrich added a comment - Thanks for your investigation and the patch, I'll try this tomorrow! Can you tell (if the patch works) how long it normally takes untill it will find its way into an offical release?
          Hide
          mgrigorov Martin Grigorov added a comment -

          I've linked this ticket to WICKET-2782 for the problem with stateless.
          But the patch seems to overcome this problem.

          Can you check your main application with the suggested patch applied ?
          I think there are no more changes needed. If the session is marked dirty then it is saved as an attribute in the http session at the end of the request.

          Show
          mgrigorov Martin Grigorov added a comment - I've linked this ticket to WICKET-2782 for the problem with stateless. But the patch seems to overcome this problem. Can you check your main application with the suggested patch applied ? I think there are no more changes needed. If the session is marked dirty then it is saved as an attribute in the http session at the end of the request.
          Hide
          laeubi Christoph Läubrich added a comment -

          > Can you please give more details ?
          Prior to WICKET-5380 setAttribute was called for each request, now it is only called once and the session is never marked as "dirty" even if the object has changed. So a similar issue might happen there, maybe it is okay if the session cache is lost under some circumstance.

          > You know sending us patches or pull requests is not forbidden
          Yes But in that very central case I think it is better to let it do someone with a more wicket-internal-view, so I hope its okay to just suggest what might be changed.

          > Adding calls to dirty causes a major problem - the session is being bound even for stateless applications
          Well... if they depend on a state (page-id counter) how can they be stateless? I'm not sure how Wicket handles/detecct this case, but maybe it would be suitable in such a case to not call the Session but a gobal static "getTempPageID" (AtomicInteger).
          Or, maybe only call dirty if the session is not temporary?

          Show
          laeubi Christoph Läubrich added a comment - > Can you please give more details ? Prior to WICKET-5380 setAttribute was called for each request, now it is only called once and the session is never marked as "dirty" even if the object has changed. So a similar issue might happen there, maybe it is okay if the session cache is lost under some circumstance. > You know sending us patches or pull requests is not forbidden Yes But in that very central case I think it is better to let it do someone with a more wicket-internal-view, so I hope its okay to just suggest what might be changed. > Adding calls to dirty causes a major problem - the session is being bound even for stateless applications Well... if they depend on a state (page-id counter) how can they be stateless? I'm not sure how Wicket handles/detecct this case, but maybe it would be suitable in such a case to not call the Session but a gobal static "getTempPageID" (AtomicInteger). Or, maybe only call dirty if the session is not temporary?
          Hide
          mgrigorov Martin Grigorov added a comment -

          This patch seems to do the job.

          Show
          mgrigorov Martin Grigorov added a comment - This patch seems to do the job.
          Hide
          mgrigorov Martin Grigorov added a comment -

          Adding calls to dirty causes a major problem - the session is being bound even for stateless applications.
          org.apache.wicket.session.BindSessionOnRedirectTest#bindSessionWhenThereAreFeedbackMessages() shows the problem.

          Show
          mgrigorov Martin Grigorov added a comment - Adding calls to dirty causes a major problem - the session is being bound even for stateless applications. org.apache.wicket.session.BindSessionOnRedirectTest#bindSessionWhenThereAreFeedbackMessages() shows the problem.
          Hide
          mgrigorov Martin Grigorov added a comment -

          > PLEASE NOTE: PageStoreManager has also an issue, since it does not mark the session dirty due to change WICKET-5380 this would be neccesary now!

          Can you please give more details ?
          By adding calls to dirty() in org.apache.wicket.Session#nextSequenceValue() and org.apache.wicket.Session#nextPageId() your quickstart starts to behave as expected.
          Is there something more to be done ?
          You know sending us patches or pull requests is not forbidden

          Show
          mgrigorov Martin Grigorov added a comment - > PLEASE NOTE: PageStoreManager has also an issue, since it does not mark the session dirty due to change WICKET-5380 this would be neccesary now! Can you please give more details ? By adding calls to dirty() in org.apache.wicket.Session#nextSequenceValue() and org.apache.wicket.Session#nextPageId() your quickstart starts to behave as expected. Is there something more to be done ? You know sending us patches or pull requests is not forbidden
          Hide
          laeubi Christoph Läubrich added a comment -

          I hope it is okay to change this to even 'critical' since this can cause data to be lost.

          From taking a look at the Code of Session, it seems these both field values have the issue:

          private int sequence = 1;
          private int pageId = 0;

          All other modification methods set the dirty flag, but these do not:

          public synchronized int nextSequenceValue() {
          		return sequence++;
          	}
          
          	public synchronized int nextPageId() {
          		return pageId++;
          	}

          I would suggest to instead use field values, simply use SessionAttributes via get/setAttribute or mark session dirty after each call.
          Maybe it would also be a good idea to switch to AtomicInteger and remove the need to syncronize here also.

          PLEASE NOTE: PageStoreManager has also an issue, since it does not mark the session dirty due to change WICKET-5380 this would be neccesary now!

          Is there any plan when this might be fixed and released? This currently require us to do a lot more requests than neccessary (and that leads to some unwanted DB access in the end) to prevent it. This leads to performance problem and thus is rather critical for us.

          Show
          laeubi Christoph Läubrich added a comment - I hope it is okay to change this to even 'critical' since this can cause data to be lost. From taking a look at the Code of Session, it seems these both field values have the issue: private int sequence = 1; private int pageId = 0; All other modification methods set the dirty flag, but these do not: public synchronized int nextSequenceValue() { return sequence++; } public synchronized int nextPageId() { return pageId++; } I would suggest to instead use field values, simply use SessionAttributes via get/setAttribute or mark session dirty after each call. Maybe it would also be a good idea to switch to AtomicInteger and remove the need to syncronize here also. PLEASE NOTE: PageStoreManager has also an issue, since it does not mark the session dirty due to change WICKET-5380 this would be neccesary now! Is there any plan when this might be fixed and released? This currently require us to do a lot more requests than neccessary (and that leads to some unwanted DB access in the end) to prevent it. This leads to performance problem and thus is rather critical for us.
          Hide
          laeubi Christoph Läubrich added a comment - - edited

          The only Workaround is to change the session on every call what is more a hack and requires code changes all over the code. (note this is also just a sideeffect that it works, if a SessionManager would decide to store not the whole session but individual attributes (as in this quickstart) it would not help...)

          Show
          laeubi Christoph Läubrich added a comment - - edited The only Workaround is to change the session on every call what is more a hack and requires code changes all over the code. (note this is also just a sideeffect that it works, if a SessionManager would decide to store not the whole session but individual attributes (as in this quickstart) it would not help...)
          Hide
          laeubi Christoph Läubrich added a comment -

          I was able to reduce the problem significantly by emulating a persitent session via the ISessionStore.

          You can find a quickstart here: https://github.com/laeubi/quickstart/tree/master/WICKET5473

          Steps to reproduce:

          • start quickstart
          • open homepage (page id will be shown as 0, instance as 0)
          • click the link (page id will be shwon as 1, instance as 1)
          • click the link again (page id will be shown as 1 instance as 1)
          • click the link again (page id will be shown as 1 instance as 1)
          • click the link again (page id will be shown as 1 instance as 2)
          • repeat as you like

          So what do you see: Two pages getting the same page id repeatedly, in the log you will see that only one call to setValue(wicket:wicket.WICKET5473:session) is performed and after then only getValue is called resulting in getting the state back from the first call.

          Example Output
          Create Page 1
          setValue(wicket:wicket.WICKET5473:session) := org.apache.wicket.protocol.http.WebSession@15f1f9c
          getValue(wicket:wicket.WICKET5473:session) := org.apache.wicket.protocol.http.WebSession@13a34af
          setValue(wicket:wicket.WICKET5473:wicket:persistentPageManagerData - wicket.WICKET5473) := org.apache.wicket.page.PageStoreManager$SessionEntry@20c906
          getValue(wicket:wicket.WICKET5473:session) := org.apache.wicket.protocol.http.WebSession@b914b3
          getValue(wicket:wicket.WICKET5473:session) := org.apache.wicket.protocol.http.WebSession@fdfc58
          getValue(wicket:wicket.WICKET5473:session) := org.apache.wicket.protocol.http.WebSession@1904e0d
          getValue(wicket:wicket.WICKET5473:session) := org.apache.wicket.protocol.http.WebSession@178920a
          getValue(wicket:wicket.WICKET5473:session) := org.apache.wicket.protocol.http.WebSession@bbfa5c
          getValue(wicket:wicket.WICKET5473:session) := org.apache.wicket.protocol.http.WebSession@131de9b
          getValue(wicket:wicket.WICKET5473:session) := org.apache.wicket.protocol.http.WebSession@f42160
          getValue(wicket:wicket.WICKET5473:session) := org.apache.wicket.protocol.http.WebSession@1bb326c
          getValue(wicket:wicket.WICKET5473:session) := org.apache.wicket.protocol.http.WebSession@eb7331
          getValue(wicket:wicket.WICKET5473:session) := org.apache.wicket.protocol.http.WebSession@7ff5b6
          getValue(wicket:wicket.WICKET5473:session) := org.apache.wicket.protocol.http.WebSession@dc9065
          getValue(wicket:wicket.WICKET5473:session) := org.apache.wicket.protocol.http.WebSession@8beff2
          getValue(wicket:wicket.WICKET5473:wicket:persistentPageManagerData - wicket.WICKET5473) := org.apache.wicket.page.PageStoreManager$SessionEntry@1362a63
          onClick Page 1
          getValue(wicket:wicket.WICKET5473:wicket:persistentPageManagerData - wicket.WICKET5473) := org.apache.wicket.page.PageStoreManager$SessionEntry@57ae58
          getValue(wicket:wicket.WICKET5473:session) := org.apache.wicket.protocol.http.WebSession@813bc1
          getValue(wicket:wicket.WICKET5473:session) := org.apache.wicket.protocol.http.WebSession@7a36a2
          Create Page 2
          getValue(wicket:wicket.WICKET5473:wicket:persistentPageManagerData - wicket.WICKET5473) := org.apache.wicket.page.PageStoreManager$SessionEntry@1b5c22f
          getValue(wicket:wicket.WICKET5473:session) := org.apache.wicket.protocol.http.WebSession@cd4544
          getValue(wicket:wicket.WICKET5473:session) := org.apache.wicket.protocol.http.WebSession@7e75d2
          getValue(wicket:wicket.WICKET5473:session) := org.apache.wicket.protocol.http.WebSession@694f12
          getValue(wicket:wicket.WICKET5473:session) := org.apache.wicket.protocol.http.WebSession@345b43
          getValue(wicket:wicket.WICKET5473:session) := org.apache.wicket.protocol.http.WebSession@153b2cb
          getValue(wicket:wicket.WICKET5473:wicket:persistentPageManagerData - wicket.WICKET5473) := org.apache.wicket.page.PageStoreManager$SessionEntry@1ff2e1b
          

          You can now change

          bugs.WicketApplication.java
          private static boolean isPersitent = true;

          in WicketApplication to

          bugs.WicketApplication.java
          private static boolean isPersitent = false;

          rerun the code and you'll see that page ids are incremented on each click since the default in-memory model is used.

          Let me know if you need any further details.

          Show
          laeubi Christoph Läubrich added a comment - I was able to reduce the problem significantly by emulating a persitent session via the ISessionStore. You can find a quickstart here: https://github.com/laeubi/quickstart/tree/master/WICKET5473 Steps to reproduce: start quickstart open homepage (page id will be shown as 0, instance as 0) click the link (page id will be shwon as 1, instance as 1) click the link again (page id will be shown as 1 instance as 1) click the link again (page id will be shown as 1 instance as 1) click the link again (page id will be shown as 1 instance as 2) repeat as you like So what do you see: Two pages getting the same page id repeatedly, in the log you will see that only one call to setValue(wicket:wicket.WICKET5473:session) is performed and after then only getValue is called resulting in getting the state back from the first call. Example Output Create Page 1 setValue(wicket:wicket.WICKET5473:session) := org.apache.wicket.protocol.http.WebSession@15f1f9c getValue(wicket:wicket.WICKET5473:session) := org.apache.wicket.protocol.http.WebSession@13a34af setValue(wicket:wicket.WICKET5473:wicket:persistentPageManagerData - wicket.WICKET5473) := org.apache.wicket.page.PageStoreManager$SessionEntry@20c906 getValue(wicket:wicket.WICKET5473:session) := org.apache.wicket.protocol.http.WebSession@b914b3 getValue(wicket:wicket.WICKET5473:session) := org.apache.wicket.protocol.http.WebSession@fdfc58 getValue(wicket:wicket.WICKET5473:session) := org.apache.wicket.protocol.http.WebSession@1904e0d getValue(wicket:wicket.WICKET5473:session) := org.apache.wicket.protocol.http.WebSession@178920a getValue(wicket:wicket.WICKET5473:session) := org.apache.wicket.protocol.http.WebSession@bbfa5c getValue(wicket:wicket.WICKET5473:session) := org.apache.wicket.protocol.http.WebSession@131de9b getValue(wicket:wicket.WICKET5473:session) := org.apache.wicket.protocol.http.WebSession@f42160 getValue(wicket:wicket.WICKET5473:session) := org.apache.wicket.protocol.http.WebSession@1bb326c getValue(wicket:wicket.WICKET5473:session) := org.apache.wicket.protocol.http.WebSession@eb7331 getValue(wicket:wicket.WICKET5473:session) := org.apache.wicket.protocol.http.WebSession@7ff5b6 getValue(wicket:wicket.WICKET5473:session) := org.apache.wicket.protocol.http.WebSession@dc9065 getValue(wicket:wicket.WICKET5473:session) := org.apache.wicket.protocol.http.WebSession@8beff2 getValue(wicket:wicket.WICKET5473:wicket:persistentPageManagerData - wicket.WICKET5473) := org.apache.wicket.page.PageStoreManager$SessionEntry@1362a63 onClick Page 1 getValue(wicket:wicket.WICKET5473:wicket:persistentPageManagerData - wicket.WICKET5473) := org.apache.wicket.page.PageStoreManager$SessionEntry@57ae58 getValue(wicket:wicket.WICKET5473:session) := org.apache.wicket.protocol.http.WebSession@813bc1 getValue(wicket:wicket.WICKET5473:session) := org.apache.wicket.protocol.http.WebSession@7a36a2 Create Page 2 getValue(wicket:wicket.WICKET5473:wicket:persistentPageManagerData - wicket.WICKET5473) := org.apache.wicket.page.PageStoreManager$SessionEntry@1b5c22f getValue(wicket:wicket.WICKET5473:session) := org.apache.wicket.protocol.http.WebSession@cd4544 getValue(wicket:wicket.WICKET5473:session) := org.apache.wicket.protocol.http.WebSession@7e75d2 getValue(wicket:wicket.WICKET5473:session) := org.apache.wicket.protocol.http.WebSession@694f12 getValue(wicket:wicket.WICKET5473:session) := org.apache.wicket.protocol.http.WebSession@345b43 getValue(wicket:wicket.WICKET5473:session) := org.apache.wicket.protocol.http.WebSession@153b2cb getValue(wicket:wicket.WICKET5473:wicket:persistentPageManagerData - wicket.WICKET5473) := org.apache.wicket.page.PageStoreManager$SessionEntry@1ff2e1b You can now change bugs.WicketApplication.java private static boolean isPersitent = true ; in WicketApplication to bugs.WicketApplication.java private static boolean isPersitent = false ; rerun the code and you'll see that page ids are incremented on each click since the default in-memory model is used. Let me know if you need any further details.
          Hide
          mgrigorov Martin Grigorov added a comment -

          Reducing the Priotity to Major because it seems you have a workaround: override #equals() in YourSession and call session.dirty() when needed.

          Show
          mgrigorov Martin Grigorov added a comment - Reducing the Priotity to Major because it seems you have a workaround: override #equals() in YourSession and call session.dirty() when needed.
          Hide
          mgrigorov Martin Grigorov added a comment -

          Wicket's Session#setAttribute() just delegates to ISessionStore#setAttribute(). Nothing more.

          The (Wicket)Session object is stored in the ISessionStore (i.e. in HttpSessionStore -> HttpSession) by org.apache.wicket.Session#internalDetach() that calls getSessionStore().flushSession(request, this); when the Session is marked as dirty.

          Show
          mgrigorov Martin Grigorov added a comment - Wicket's Session#setAttribute() just delegates to ISessionStore#setAttribute(). Nothing more. The (Wicket)Session object is stored in the ISessionStore (i.e. in HttpSessionStore -> HttpSession) by org.apache.wicket.Session#internalDetach() that calls getSessionStore().flushSession(request, this); when the Session is marked as dirty.
          Hide
          laeubi Christoph Läubrich added a comment - - edited

          I'll try but fear it will take some time, maybe tomorrow.
          Maybe you nevertheless want to take a look at (Wicket)Session and add these simple log statements:

          • one logging setAttribute
          • one logging getAttribute
          • one on nextPageID

          then use an arbitrary Wicket app, open some pages. And then look at the sequence generated under the assumption that all state that is changed without "setAttribute" is called will be lost. It should be perfectly clear than that there is an issue.

          Show
          laeubi Christoph Läubrich added a comment - - edited I'll try but fear it will take some time, maybe tomorrow. Maybe you nevertheless want to take a look at (Wicket)Session and add these simple log statements: one logging setAttribute one logging getAttribute one on nextPageID then use an arbitrary Wicket app, open some pages. And then look at the sequence generated under the assumption that all state that is changed without "setAttribute" is called will be lost. It should be perfectly clear than that there is an issue.
          Hide
          mgrigorov Martin Grigorov added a comment -

          Without a quickstart and steps to reproduce it will be hard to improve it.
          Please take some time to prove that it is broken.

          Show
          mgrigorov Martin Grigorov added a comment - Without a quickstart and steps to reproduce it will be hard to improve it. Please take some time to prove that it is broken.
          Hide
          laeubi Christoph Läubrich added a comment -

          Maybe the following would work

          • custom wicket with log output in setIntegerId (not 100% sure how the mehtod is called....)
          • Wicket with two pages each with a link to the other via a dynamic link
          • create a custom pagestore that do not use the session (e.g. just a simple Hashmap in memory)
          • jetty 8 with JDBC Session store and default settings
          • enable log output for JDBC sessionmanager
          • open page one
          • open page two
          • close one of them and reopen it...
          • you would see log output with pageid 1 + 2
          • repeat untill you are at page id 10 or something
          • wait until JDBC manager reloads the session from db
          • you will see old page ID assigned and due to a bug (see https://bugs.eclipse.org/bugs/show_bug.cgi?id=425930) since session is reloaded on each request.

          It is rather complex to setup (and debug!) since you need the following to be true:

          • have a Pagemanager/Store that do not alter the session
          • have a HttpSession that uses serialization and reloads data from serilized form (e.g. on a regular basis or on restart)
          Show
          laeubi Christoph Läubrich added a comment - Maybe the following would work custom wicket with log output in setIntegerId (not 100% sure how the mehtod is called....) Wicket with two pages each with a link to the other via a dynamic link create a custom pagestore that do not use the session (e.g. just a simple Hashmap in memory) jetty 8 with JDBC Session store and default settings enable log output for JDBC sessionmanager open page one open page two close one of them and reopen it... you would see log output with pageid 1 + 2 repeat untill you are at page id 10 or something wait until JDBC manager reloads the session from db you will see old page ID assigned and due to a bug (see https://bugs.eclipse.org/bugs/show_bug.cgi?id=425930 ) since session is reloaded on each request. It is rather complex to setup (and debug!) since you need the following to be true: have a Pagemanager/Store that do not alter the session have a HttpSession that uses serialization and reloads data from serilized form (e.g. on a regular basis or on restart)
          Hide
          mgrigorov Martin Grigorov added a comment -

          Can you provide a quickstart that demonstrates the problem ?

          Show
          mgrigorov Martin Grigorov added a comment - Can you provide a quickstart that demonstrates the problem ?
          Hide
          laeubi Christoph Läubrich added a comment -

          This is related to WICKET-5390 wich was not a complete fix for session issues in presence with a non memmory-session managment. Since I can't reopen the issue for some reason, and there are other issues not only with jettys JDBCSessionmanager but any sessionmanager that might persist and restore session data later.

          Show
          laeubi Christoph Läubrich added a comment - This is related to WICKET-5390 wich was not a complete fix for session issues in presence with a non memmory-session managment. Since I can't reopen the issue for some reason, and there are other issues not only with jettys JDBCSessionmanager but any sessionmanager that might persist and restore session data later.

            People

            • Assignee:
              mgrigorov Martin Grigorov
              Reporter:
              laeubi Christoph Läubrich
            • Votes:
              1 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development